smartcar / python-sdk

Smartcar Python SDK
MIT License
44 stars 13 forks source link

feat: Add Fuel and EV methods #58

Closed KarthikBhaskara closed 5 years ago

KyleKaniecki commented 5 years ago

I know this is sorta out of the scope of this review, but I figured we could start the conversation now since I saw some things that make me cringe...

What do you guys think about moving over to black's formatting method instead of default PEP8? A lot of big name python projects are moving over to it because it just makes sense (SQLAlchemy, Django, Django Channels, PyPA applications [virtualenv, pipenv, etc])

The reason I say is because it does drop a lot of the stupid python linting styles like this here. It also follows a lot of what YAPF does, with some minor changes. For example, it would format with a little bit more sense like such as this function call:

self.api.set_unit_system(
    'metric' if unit_system == 'metric' else 'imperial')

to something a little more readable and easy to follow:

self.api.set_unit_system(
    'metric' if unit_system == 'metric' else 'imperial'
)

Here, you can see exactly where the function call ends. With the default PEP8, you need to look for trailing parentheses at the end of lines. Personally, I hate how it looks and feels.

And multi-lines like

self.assertRaisesRegexp(exception, self.EXPECTED,
                        smartcar.requester.call, 'GET', self.URL)

To something more readable and grep-able like the following:

self.assertRaisesRegexp(
    exception,
    self.EXPECTED,
    smartcar.requester.call,
    'GET',
    self.URL
)

Instead of the indentation hell that PEP8 wants them to be. This would make method calls more grep-able and easy to read, since they would be formatted just like multi-line dictionaries and the like.

return {
    'data': response.json(),
    'unit_system': response.headers['sc-unit-system'],
    'age': dateutil.parser.parse(response.headers['sc-data-age']),
}

Thoughts?

KarthikBhaskara commented 5 years ago

makes sense. we should decide on linting rules across all our SDKs and implement separately. Feel free to set up some time on the calendar. I know @morgannewman would love to talk about this