Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mixing pythonic variable_names with CamelCase #217

Closed
jlafon opened this issue Jan 20, 2014 · 4 comments
Closed

Mixing pythonic variable_names with CamelCase #217

jlafon opened this issue Jan 20, 2014 · 4 comments

Comments

@jlafon
Copy link
Contributor

jlafon commented Jan 20, 2014

I've been writing a wrapper around botocore for interfacing with DynamoDB, and have discovered that it requires a weird mix of parameters names. When passing in key word arguments, it seems that top level keys must be pythonic, while lower level keys must be camel cased.

For example, these arguments are not valid when used with UpdateTable:

update_table_operation({
    'provisioned_throughput': {
        'write_capacity_units': 2, 
        'read_capacity_units': 2
    }, 
    'table_name': 'test-table'
})
botocore.exceptions.UnknownKeyError: Unknown key 'read_capacity_units' for param 'ProvisionedThroughput'.  Must be one of: ReadCapacityUnits, WriteCapacityUnits

This mixed syntax works:

update_table_operation({
    'provisioned_throughput': {
        'WriteCapacityUnits': 2, 
        'ReadCapacityUnits': 2
    }, 
    'table_name': 'test-table'
})

Note that changing the top level key word to camel case does not work either.

The exception is raised here:

def _validate_known_keys(self, value):
        valid_keys = [p.name for p in self.members]
        for key in value:
            if key not in valid_keys:
                raise UnknownKeyError(
                    value=key, choices=', '.join(valid_keys), param=self.name)

The valid_keys list is constructed based on the camel case names for parameters, but each object in self.members also has a py_name attribute. In fact, on the ReadCapacityUnits member object the attribute is defined as read_capacity_units. It would be simple enough to build the list using [p.py_name for p in self.members], but I'm guessing there are many other places where a similar list is constructed.

If a purely pythonic (or camel case) API is desired, I am willing to work on it and send a PR.

@disruptek
Copy link

I've also found this behavior unintuitive in the extreme; to me, it's a gross design flaw that will doubtless bite user after user.

Considering my MWS/FPS code, it's clear that my preference is to make our "thin layer" match the Amazon API as closely as possible, including choice of parameter name and case. In fact, it was a almost a concession on my part to name the call methods in in snake_case versus the Amazon convention of CapWords (and it of course meant additional needless complexity in the code as well).

Despite the fact that MWS/FPS likely differs from every other boto module in this regard, I don't recall fielding a single question about argument case; once users understand that the API offers a 1:1 translation, they can rely upon that fact across the entire module and leverage both Amazon and boto documentation. There's some real value to this convention that should not be overlooked; it may appear to be a trivial style change, but to the user, if it looks the same, it's probably the same -- and if it looks different, perhaps it is different. Don't make me think!

In this case, I feel that the slavish adoption of an apparent pythonic convention has already meant a significant sacrifice to the translucency of the API, but the fact that the convention varies inexplicably inside input data structures is, frankly, mind-boggling.

As far as method argument case goes, PEP8 offers no specific recommendation on convention, but to quote the most relevant section of that document specifically:

Naming Conventions

Overriding Principle

Names that are visible to the user as public parts of the API should follow conventions that reflect usage rather than implementation.

I don't know what else needs to be said, except to restate that any convention that's followed consistently would be preferable to what we have to work with at present.

@jlafon
Copy link
Contributor Author

jlafon commented Jan 31, 2014

I've written my wrapper around botocore now, available here. Because of this mix of camel case and snake case variable names I have resorted to using a regex to transform the names.

Speaking specifically about the DynamoDB API, there are quite a few strings used throughout it (here is a list of them). That leaves me with the choice of defining a snake case and a camel case version of every single constant, or using a regex to go back and forth. I don't really like either, because it decreases the readability of the code, and I have to remember when to use snake case and when to use camel case. Although I'm using DynamoDB as an example, the same can be said about any of the services wrapped by botocore.

In summary, in order to build something consistent for our users, we're stuck awkwardly adopting either camel case or snake case ourselves and translating where appropriately in our own abstractions. Boto and botocore are immensely valuable resources to the Python community, being amongst the most popular packages. You should take advantage of your reach to demonstrate, by example, how to write beautiful Python.

@jamesls
Copy link
Member

jamesls commented Jan 31, 2014

@jlafon Thanks for the feedback and sorry for the delayed response.

I think you make some valid points. The original motivation for the snake_casing of top level parameters was that if users are using botocore like this:

s3 = session.get_service('s3')
list_objects = s3.get_operation('ListObjects')
list_objects.call(endpoint, bucket='jamesls-test-sync', max_keys=1)

Then the API does not mix any casing. The idea was to treat the arguments as .call(endpoint, arg_name1=<anything>, arg_name2=<anything>, arg_name3=<anything>).

However, as you point out when <anything> happens to be a dict, then the casing reflects that used by the API, and for AWS APIs, that always means camelCase or CamelCase.

I would not be opposed to changing this, but with a few caveats:

  • We'd like to avoid breaking existing users of botocore (or at least minimal breakage)
  • I think we'd still like to support the exact casing used by the service API (the aws-cli, for example, uses this service API casing, e.g. --provisioned-throughput ReadCapacityUnits=10,WriteCapacityUnits=10

It might be as straightforward as relaxing the validation on the _known_keys check, as you suggest, but I'd have to look more in depth to be sure.

btw, pynamodb looks like a cool project.

danielgtaylor added a commit that referenced this issue Mar 10, 2014
Consistent casing for parameters. Fixes #240, #217.
@disruptek
Copy link

Thanks very much, guys, this is a nice improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants