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

add flake8 in build check, reorder imports #17

Merged
merged 1 commit into from Aug 13, 2016
Merged

add flake8 in build check, reorder imports #17

merged 1 commit into from Aug 13, 2016

Conversation

kelvintaywl
Copy link
Contributor

This PR updates the SDK such that:

  1. introduce flake8 as part of Travis CI build to ensure that we follow PEP8 guidelines as close as possible.
  2. reordering of imports to follow import guidelines from PEP8 (ref: https://www.python.org/dev/peps/pep-0008/#imports).

I have explicitly set flake8 config to ignore max line length (defaults at 79 chars) though. perhaps we want to enforce a specific max line length?

@ardydedase
Copy link
Contributor

Hi @kelvintaywl,

Let's stick to the standard 79 characters.
Also, the build has failed. Could it be because it failed the flake8 check?

Thanks for this!

Regards,
Ardy

Ensure that flake8 will run against committed code for all builds.
This is so that we try to keep to PEP8 recommendations.

Code changes include:
    -  reformatting of code to fit 79 char limits per line
    -  ignoring a specific E402 PEP8 rule regarding module imports at top of file
@coveralls
Copy link

coveralls commented Aug 11, 2016

Coverage Status

Coverage decreased (-0.9%) to 92.308% when pulling b446dee on kelvintaywl:add-flake8-reorder-imports into 0dd7eec on Skyscanner:master.

@kelvintaywl
Copy link
Contributor Author

kelvintaywl commented Aug 11, 2016

Hi @ardydedase

Thanks for the response!
I have set it to follow the recommended 79 char limitation then.
As a result, I have also formatted the codes to fit the 79-char limitation.

A small note, however, as I have elected to ignore the E402 rule from PEP8 as there seem to be a conflict between flake8's handling of checking E402 against dunder names such as __author__, __version__, etc.

build is currently failing as flake8 does not support python 2.6. I'm not sure if there's a strong motivation to support python 2.6 though.

@ardydedase
Copy link
Contributor

Hi @kelvintaywl,

The changes are looking good. Noted the E402 rule as well.
I'm now considering ditching Python 2.6. It also causes us syntax problems sometimes.
Thanks a lot for your contribution!

Regards,
Ardy

@ardydedase ardydedase merged commit 65f002b into Skyscanner:master Aug 13, 2016
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

Successfully merging this pull request may close these issues.

None yet

3 participants