Skip to content

[PEP8] fixed PEP 8 #1432

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

Closed
wants to merge 1 commit into from
Closed

Conversation

goffi-contrib
Copy link
Contributor

Tests are currently failing because of PEP 8 issues. This patch fixes
them and flake8 is now passing.
Reformatting has been done using "black" automated tool.

Tests are currently failing because of PEP 8 issues. This patch fixes
them and flake8 is now passing.
Reformatting has been done using "black" automated tool.
@goffi-contrib
Copy link
Contributor Author

Note: I have reformatted only the files causing trouble, and disabled string normalization, this is to avoid modifying too many files, and breaking other pull requests.

@AndreMiras
Copy link
Member

AndreMiras commented Oct 24, 2018

Thank you very much. Well it would at least conflict with #1412 from @inclement I suppose 😅
Edit:
Actually I just checked, indeed linter is failing very recently, but it's because of the recent flake8==3.6.0 update, some rule interpretation changed.

@goffi-contrib
Copy link
Contributor Author

@AndreMiras it should not be such a big deal to rebase after this PR, the lines are not modified a lot.

@inclement
Copy link
Member

I'd rather not merge this without having an overall decision of whether we want to use black on everything, or not. Some of black's code style choices are quite distinctive and different to what p4a tends to do at the moment.

Personally, I'm not sure I'm a fan of some of these choices. In particular, black optimises for future patch simplicity over current code vertical conciseness, it is very eager to move brackets to new lines on their own even where this makes the code much taller than really necessary.

I'm not dismissing black out of hand, in fact I've raised the possibility of using it more than once myself on irc/discord, I'd just rather not half use it this one time to generate a lot of code line changes that may not get maintained and aren't consistent with the rest of the code base.

(Also this diff reveals some minor problems that would be nice to fix manually if we do run black, like the way it changes things like 'some string'\n' some other string' to 'some string' 'some other string' when the strings should actually just be fully merged if they're going to be on one line.)

@goffi-contrib
Copy link
Contributor Author

@inclement it makes sense. Anyway in a previous PR which has been merged, the style issues blocking the tests have been ignored, so tests are running again, and this was my main goal.

So no problem for me if you want to close this PR and discuss style changes more extensively. I also think if an automated tool/code formatting is done, it should be done on the whole code base, and not just the few files causing trouble, like I did here.

@inclement
Copy link
Member

Okay, I'll close this PR for that reason. Thanks for making it though, and this doesn't mean we can't discuss using something like black.

@inclement inclement closed this Oct 27, 2018
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.

3 participants