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

Tidy Code To Follow PEP8 and work with FLAKE8 #106

Closed
wants to merge 4 commits into from

Conversation

Richienb
Copy link
Contributor

@Richienb Richienb commented Jul 7, 2018

Tidied the code to follow the PEP8 specification and FLAKE8.

@Stewori
Copy link
Contributor

Stewori commented Jul 7, 2018

Hey, thanks for this work!
I would - however - not want it to be merged right now because it would complicate merging of other PRs that might get into 2.7.2. Another thing is merging Jython 3 sandbox on top of Jython 2.7.2, which is complicated enough already.
I suggest we apply this work as the last step before releasing Jython 2.7.2, such that there are no crucial PRs applied on top. Then it would be easy to fork a de-facto Jython 2.7.2 equivalent without missing further PRs. That fork can then be used to experiment with Jython 3 merging.

@Richienb
Copy link
Contributor Author

Richienb commented Jul 8, 2018

Ok! Give me a heads up before the 2.7.2 release so I can re-tidy the code.

@jeff5
Copy link
Member

jeff5 commented Jul 10, 2018

Hi @Richienb, thanks for this PR. I don't believe we can accept this as it is.

I would guess this has been done with autopep8 or similar, as it's an inhuman amount of work otherwise. The difficulty with automating "style" is that machines have very little sense of what conveys meaning best to humans.

Humans, however, have to review things before we commit them. PRs that take this in smaller chunks are more likely to be reviewed.

At first I thought this was all white space change, which ought to be harmless, and I started to write a reply about where we should and shouldn't be glad of such style adjustments (mostly ok in our Jython-specific code, not ok in code we have borrowed). However I noticed that the tool hasn't confined itself to white space, and has done some things that make me wonder if you are processing the code as Python 3. Here are examples:

  1. This breaks the test:
    Richienb@b290cdd#diff-b5a4e13e358d81ad5390bc49791708bbL218
    because 1L and 1 are different things in Python 2.

  2. It changes has_key to in e.g. here:
    b290cdd#diff-9a99d95dcbb940586db8d2adaf741581L94
    which is normally a sensible thing, but maybe we intend explicitly to call has_key which is still present in Python 2.7.

  3. sys.maxint is definitely a thing in Python 2:
    b290cdd#diff-efb1fc502b72bbe0a7f4babcae5d6e00L242

There are lots more. So we definitely couldn't commit this as it is.

Back with the question of style, some of this change is good. In particular, I think this does a lot of good to the readability of the demo material, fixing indents and removing the hard tabs, and looks harmless. (Some of these demos don't work anymore, and maybe should go, but that's another issue.)

It is worth reading this: PyCQA/pycodestyle#466, and what PEP 8 itself has to say about the hobgoblin of small minds. Machines are great at consistency, which often helps readability. PEP 8 allows a range of styles. This is enforcing a certain set of rules, that are somebody's, yours or the tool authors'. It's not always an improvement. This is definitely less readable:
b290cdd#diff-980c1b657f34c4b3557c837f4fc15f48L132
And people should be allowed to use lambda if they want to:
b290cdd#diff-2a3bf3d8a3ed8dfece743aa494329e53L1378

Anything in the lib-python tree is a direct copy of the CPython stdlib and we don't change it. If you can read the Ant build.xmlyou may see how it is used. A fair amount of other material under Lib is adapted from CPython or other sources and we would like to keep the parts we didn't change identical to the original (so we can see our bits with diff, and pull in updates).

Here's what I think might be useful if I haven't put you off contributing ...

If you could clarify the Python 2/3 question (see Flake8 manual), a PR focusing just on the demo material is likely to be accepted easily.

PRs addressing files ending _jy, or that are otherwise original to Jython (bug tests, no counterpart in lib-python or another project) are a fair target, but you and we would have to read and run the code to check nothing was broken. So, not too many files per PR (max 12 say), as one bad apple spoils the batch, and take time to understand what code is for. It would be best if you coiuld mostly avoid breaking over lines those things that are already quite readable in one, which seems to happen at the moment.

@Stewori
Copy link
Contributor

Stewori commented Jul 10, 2018

Thanks Jeff, for having a closer look than me. When I wrote my reply I had only skimmed through the first few files, which were the demos and looked quite good. What you say makes perfect sense.

@Richienb
Copy link
Contributor Author

First of all, yes I did use AutoPEP8 and Autoflake. Thanks for telling me what's wrong. I'll work on a more suitable fix very, very soon.

@jeff5
Copy link
Member

jeff5 commented Jul 12, 2018

Thanks @Richienb. I think you have a good scope for the PR now and I'll review it, merge it if I can.

We don't (yet) manage Jython on GitHub, only mirror a Mercurial repository. So I'll export this as a patch, apply it there, and it will come back to you as a single change set on the Jython master.

I will have to tinker with it because something slightly odd has gone on in this PR such that it forward-and-reverse patches a lot of files, and in the process draws in some changes others have made to Jython since your fork. Maybe you copied files from a tip checkout into your own repo as a way of reverting? Git has better ways to manage this using branches, but I won't try to remember them here: I always look in the Git Book before I do anything more complicated than a simple commit.

Anyway, the usual thing to do after forking is to create a branch named after the issue/PR, which makes it easier to abandon or start over.

If my tinkering is not successful I'll ask you to re-work it that way (as a new PR). If you fancy doing similar work on other groups of our Python files, you'd be welcome, but please take that approach: a named branch starting at a change set we have in common (not one only on your current "master"), and a new PR.

@Richienb
Copy link
Contributor Author

The commit I just created is to resolve a file conflict.

@jeff5
Copy link
Member

jeff5 commented Jul 12, 2018

@Richienb: In the end, it was easiest to merge the Demo folder from your repo and credit you in my check-in. You are now immortalised at:
https://hg.python.org/jython/rev/c532fa336c22
I tried a couple of the swing demos to make sure they still work. (Others claim not to work, but if they're worth keeping at all, they may as well be pretty.)

Thanks again.

@jeff5 jeff5 closed this Jul 12, 2018
@Richienb
Copy link
Contributor Author

I'm happy to be a contributor to Jython. Thank you.

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