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

PEP8 and minor fixes #115

Closed
wants to merge 4 commits into from
Closed

PEP8 and minor fixes #115

wants to merge 4 commits into from

Conversation

kamichal
Copy link

@kamichal kamichal commented Aug 20, 2018

https://www.python.org/dev/peps/pep-0008/

One of Guido's key insights is that code is read much more often than it is written. The guidelines provided here are intended to improve the readability of code and make it consistent across the wide spectrum of Python code. As PEP 20 says, "Readability counts".

The code becomes formatted in such way that autoformatters like autopep8.py does not affect it (finds the code style clean).

Actions:

  • applied PEP8
  • arranged imports, removed wildcard and unused imports
  • fixed unused variables and names colliding with builit-ins (e.g. id, buffer, bytes, file, str, etc..)
  • removed tabbed indentation

Next step is to add tox running pytest on supported python interpreters and flake8 linter.

@kamichal kamichal force-pushed the master branch 2 times, most recently from 02588a2 to f7e5d18 Compare August 20, 2018 21:20
@alistair23
Copy link
Collaborator

Hey Michal,

I am happy to review this, but first do you mind rebasing it on the current master?

@kamichal
Copy link
Author

kamichal commented Nov 5, 2018

Sure, that's of course not a problem. Or rather: it's my duty to make it "mergable" with master without any conflicts. The only one obstacle is my free time. I think I'll manage it in several days.

@alistair23
Copy link
Collaborator

No worries. If possible it would be nice to have it split up into smaller pull requests. It just makes the review easier. So don't feel like you need to do it all in one go.

Michał Kaczmarczyk added 2 commits February 27, 2019 00:18
…and significant code style violations (e.g. shadowing name from, defining unused parameters, violating naming conventions, etc..).
@kamichal
Copy link
Author

OK I brought it into 'mergable to master' state. This PR is ready to be review, although it's mainly only white-space change. Now it's clean as a "well written prose" :)
BTW I'm sorry it took so long.

@alistair23
Copy link
Collaborator

Is it posible to split the PEP patch into smaller ones? It's really hard to review as a single patch. If you can't do this I can do it for you. It should be fairly straight forward to split it up.

Also can you consolidate the other changes into a single patch? Why do you need to change the Travis file?

@kamichal
Copy link
Author

kamichal commented Feb 27, 2019

Is it posible to split the PEP patch into smaller ones? It's really hard to review as a single patch.

Hey, I know it's large, but it takes just 30 minutes to review it all. All of these are whitespace changes so the review is just scrolling down. These all are unrelated changes, so you can do a few-days break in reviewing and then come back forgiving everything you read before.

Also can you consolidate the other changes into a single patch?

I removed all the commits from august 2018 and started from current master's branch HEAD.

Why do you need to change the Travis file?

I added tox configuration, it's a test automation tool that cares for installing all dependencies in separate sterile virtual environments for each interpreter defined: python 2.7, 3.4, 3.5, 3.6, 3.7 and also pypy. It runs pytest and flake8 (linter). It checks if the tested package is well configured for installation. To use it you just need to call tox in main directory. It cares for everything, so that you don't need to commit/push/wait for Travis results - you can run all of it locally on your machine.
I reconfigured travis to call call tox under-hood. It tests more than plain pytest.

@alistair23
Copy link
Collaborator

I have tidied up the travis changes as the three different commits changing the file is very hard to understand. I also split out the white space changes to make it easier to read and review. How does this branch look? https://github.com/alistair23/python-OBD/tree/alistair/pep-fixes

@kamichal
Copy link
Author

You forgot to remove stages definition from the yml. Seems I have a trouble understanding your intentions. I don't know how you do a review, but I ussually analyze diff between two branches, not for separate commits, e.g. with creating github's Pull Request, as this one: https://github.com/brendan-w/python-OBD/pull/115/files. This view combines each commit into one big diff between two branches. Github makes it for you.

@alistair23
Copy link
Collaborator

The problem with just looking at the diff is then when the commits are merged you end up with a broken history. The CI breaks in the middle of your series which will make bisecting difficult. It also makes it difficult to revert if need be.

@alistair23
Copy link
Collaborator

It also ends up being confusing with multiple commits conflicting with each other. For example your series would check in the stages section in the travis.yml file, but apparently you didn't mean to.

@alistair23 alistair23 closed this Mar 1, 2019
@alistair23 alistair23 mentioned this pull request Mar 1, 2019
@alistair23
Copy link
Collaborator

After some more research I have created hopefully the final PR for the PEP8 fixes. It is all based on your work.

Can you please review the PR #141 and let me know if that works for you? You should be able to use GitHub to review the new PR. I have marked you as an author of the commits.

I am sorry this has been such a hassle. Part of it is my lack of knowledge about PEP8. In future this process will be easier if you have clearly defined commits that can be cleanly applied and reviewed.

@kamichal
Copy link
Author

kamichal commented Mar 1, 2019

I reviewed the PR #141. It's ok. I added a few comments clarifying why is it done this way.

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

2 participants