Opened 8 years ago

Closed 8 years ago

#16216 closed enhancement (fixed)

Improve PEP8 compliance: replace "== None" by "is None"

Reported by: wluebbe Owned by:
Priority: minor Milestone: sage-6.3
Component: build Keywords: pep8
Cc: tscrim Merged in:
Authors: Wilfried Luebbe Reviewers: Ralf Stephan
Report Upstream: N/A Work issues:
Branch: 564c73c (Commits, GitHub, GitLab) Commit: 564c73c8cab8aef1ec1c9e626a8e280ea8217c8f
Dependencies: Stopgaps:

Status badges

Description

To cite from PEP8:
Comparisons to singletons like None should always be done with is or is not, never the equality operators.

This ticket is also related to a promise in ticket:15984.

Attachments (1)

patch-isNone.py (1.2 KB) - added by wluebbe 8 years ago.
Update for .rst and to exclude selected modules

Download all attachments as: .zip

Change History (35)

comment:1 Changed 8 years ago by wluebbe

  • Authors set to Wilfried Luebbe
  • Branch set to u/wluebbe/ticket/16216
  • Cc tscrim added
  • Commit set to 166bc6c497e13febf34e7f64cd947d3bd2dc99c3
  • Status changed from new to needs_review

Hi Travis, these are the changes you suggested in ticket:15984.
Would you like to review this ticket?


New commits:

83f3f2dgenerated changes from "== None" to "is None"
166bc6csome manual changes to further improve style

comment:2 Changed 8 years ago by vdelecroix

Why not provide a script here? It should be possible to apply it just before the stable release.

With a simple grep I found many more instances in

  • sage/coding/code_bounds.py
  • sage/schemes/toric/variety.py
  • sage/schemes/toric/chow_group.py
  • sage/schemes/projective/projective_morphism.py
  • sage/schemes/generic/divisor_group.py
  • sage/schemes/elliptic_curves/weierstrass_morphism.py
  • sage/schemes/elliptic_curves/ell_finite_field.py
  • sage/schemes/elliptic_curves/ell_generic.py
  • sage/geometry/toric_lattice.py
  • sage/geometry/triangulation/point_configuration.py
  • sage/geometry/polyhedron/misc.py
  • sage/geometry/polyhedron/cdd_file_format.py
  • sage/geometry/polyhedron/face.py
  • sage/numerical/optimize.py
  • sage/calculus/desolvers.py
  • sage/sandpiles/sandpile.py
  • sage/graphs/generic_graph.py
  • sage/graphs/graph_coloring.py
  • sage/graphs/graph.py
  • sage/modular/modform/ambient_eps.py
  • sage/categories/coxeter_groups.py
  • sage/matrix/constructor.py
  • sage/quadratic_forms/quadratic_formternary_Tornaria.py
  • sage/interfaces/singular.py
  • sage/combinat/root_system/root_lattice_realizations.py
  • sage/combinat/sf/ns_macdonald.py
  • sage/combinat/tableau_tuple.py
  • sage/combinat/composition_tableau.py
  • sage/combinat/k_tableau.py
  • sage/combinat/affine_permutation.py
  • sage/ext/gen_interpreters.py
Last edited 8 years ago by vdelecroix (previous) (diff)

comment:3 Changed 8 years ago by vdelecroix

I guess something like the following would do the job (trickier than what I thought... I edited twice my answer)

#!/usr/bin/env bash

for f in $(find . -name "*py");
do
    echo "processing $f"
    sed -i -e's/==\ *None$/is None/g' $f
    sed -i -e's/==\ *None\([ :)]\)/is None\1/g' $f
    sed -i -e's/!=\ *None$/is not None/g' $f
    sed -i -e's/!=\ *None\([ :)]\)/is not None\1/g' $f
done
Last edited 8 years ago by vdelecroix (previous) (diff)

comment:4 Changed 8 years ago by wluebbe

I did it with a small Python script, but instead of "\w*" I had "\w+" :-(
Wait a minute ... ;-)

comment:5 Changed 8 years ago by vdelecroix

In that thread on sage-devel I asked the question of whether we can provide a script rather than a branch... it would make more sense here.

Vincent

comment:6 Changed 8 years ago by vdelecroix

See also this thread

comment:7 Changed 8 years ago by leif

Such scripts should ideally be applied by the release manager right before doing a new stable release. Not sure whether Volker is aware of this ticket.

For new code / changes, it's up to the reviewers to check no new instances get introduced. ;-)

comment:8 Changed 8 years ago by vdelecroix

Thanks Leif. I will ask Volker directly if he does not answer on sage-devel.

The script was not the good one (because of things like "my_variable!=None"), new try

#!/usr/bin/env bash

for f in $(find . -name "*py");
do
    echo "processing $f"
    sed -i -e's/\ *==\ *None$/ is None/g' $f
    sed -i -e's/\ *==\ *None\([ :)]\)/ is None\1/g' $f
    sed -i -e's/\ *!=\ *None$/ is not None/g' $f
    sed -i -e's/\ *!=\ *None\([ :)]\)/ is not None\1/g' $f
done

I applied it to Sage source code, and obtain 505 occurrences... and Sage even starts after that!!

comment:9 Changed 8 years ago by wluebbe

There are several competing / overlapping tools that could support such a "project". Here are some links:

The cross tool mailing list is code-quality.

These are the Python guidelines:

I had a quick look at them - there is no obvious BEST one. But it would certainly be worthwhile to select / combine / customize them for use with Sage.

comment:10 Changed 8 years ago by git

  • Commit changed from 166bc6c497e13febf34e7f64cd947d3bd2dc99c3 to 05ae6c01d83821d60b76bf5a1aa7b2bca745ad71

Branch pushed to git repo; I updated commit sha1. New commits:

05ae6c0generated patches missing from first run

comment:11 follow-up: Changed 8 years ago by vdelecroix

1) The script can be done with only one call to sed and is much faster that way (took less than 10 secs on my computer)

#!/usr/bin/env bash

for f in $(find . -name "*py");
do
    echo "processing $f"
    sed -i -e's/\ *==\ *None$/ is None/g' \
           -e's/\ *==\ *None\([ :)]\)/ is None\1/g' \
           -e's/\ *!=\ *None$/ is not None/g' \
           -e's/\ *!=\ *None\([ :)]\)/ is not None\1/g' $f
done

2) There are few things to fix by hand. For example, some occurrences appear in the docstrings and the script did bad to them (src/sage/ext/gen_interpreters.py, src/sage/geometry/polyhedron/misc.py, ...)

For Wilfried:

3) It is not a good idea to provide a git branch (it will certainly create plenty of conflicts). The best would be to apply the script just before the stable release.

4) If you used a script different from mine, it would be nice that you provide it!

comment:12 Changed 8 years ago by wluebbe

The update also includes some pyx modules. And a few py modules not in your list.

The regexp now looks like

  c_pat_1 = re.compile(r"""\s? == \s* None \b""", re.VERBOSE)
  contents_1, n1 = re.subn(c_pat_1, " is None", contents_0)
  c_pat_2 = re.compile(r"""\s? != \s* None \b""", re.VERBOSE)
  new_contents, n2 = re.subn(c_pat_2, " is not None", contents_1)

comment:13 in reply to: ↑ 11 Changed 8 years ago by wluebbe

Replying to vdelecroix:

3) It is not a good idea to provide a git branch (it will certainly create plenty of conflicts). The best would be to apply the script just before the stable release.

I would agree but at least once you have to clean up the existing stuff. - And the conflicts should be manageable.

4) If you used a script different from mine, it would be nice that you provide it!

I am more familiar with Python than with the classic Un*x tools like grep and sed :-/
I attached my quick and dirty Python script.

comment:14 Changed 8 years ago by leif

There's also at least one occurrence in .rst source (as opposed to generated) files:

src/doc/en/thematic_tutorials/lie/crystals.rst:421:    sage: v.f(1).f(1) == None

comment:15 Changed 8 years ago by git

  • Commit changed from 05ae6c01d83821d60b76bf5a1aa7b2bca745ad71 to 1219d56b731119122356714ed4d26ae33c63cd5d

Branch pushed to git repo; I updated commit sha1. New commits:

1219d56one affected .rst file changed

comment:16 Changed 8 years ago by wluebbe

This was also the only .rst I could find.

Changed 8 years ago by wluebbe

Update for .rst and to exclude selected modules

comment:17 follow-up: Changed 8 years ago by vdelecroix

Hi,

Thanks Wilfried for providing your script. Your regexps look much nicer than mine!

The syntax for regexp in sed is (almost) the same as in Python! You can write what you did in sed as

sed -e's/\s\?==\s*None\b/ is None/g' \
    -e's/\s\?!=\s*None\b/ is not None/g' my_file.py

Instead of the for loop, it is faster and cleaner to use xargs. The following one-line command takes care of py and pyx files.

find -regex ".*pyx?" | xargs sed -i        \
        -e's/\s\?==\s*None\b/ is None/g'   \
        -e's/\s\?!=\s*None\b/ is not None/g'

With the command above I got 637 instances on 6.2.rc0.

At the very last, there are two possibilities to avoid occurrence of "bla == None" in the future (see Volker's answer in that thread):

  • provide a pre-commit hook for git
  • modify the "sage -b" command

Vincent

PS: sed is not Unix but GNU!

comment:18 Changed 8 years ago by wluebbe

Thanks Vincent, I guess I have to learn these tools :-))

But I am also still learning git, vim, Linux ... and often even Python. Not to mention Sage!!

comment:19 follow-up: Changed 8 years ago by tscrim

Also there is no difference in conflicts with a branch that gets merged into develop versus a script that fixes these things right before release. However it would be nice to have such a script in order to make reviews easier (mostly for reviewers who are unaware of what we're trying to do) and so we don't (re)introduce non-conformance.

Let me know if you need me to review anything (it seems both of you [Vincent and Wilfried] are taking care of it).

comment:20 in reply to: ↑ 17 Changed 8 years ago by leif

Replying to vdelecroix:

Instead of the for loop, it is faster and cleaner to use xargs. The following one-line command takes care of py and pyx files.

find -regex ".*pyx?" | xargs sed -i        \
        -e's/\s\?==\s*None\b/ is None/g'   \
        -e's/\s\?!=\s*None\b/ is not None/g'

-regex is not portable; one should use -name \*.py -o -name \*.pyx [-o -name \*.rst] instead. (And you can also use sed ... $(find ...), or -- preferably -- find ... -exec sed ..., because the latter reduces the required size of the environment, i.e. argv.)

sed -i ... is (unfortunately) also not portable, nor is \s, and IIRC neither \b nor \?.


PS: sed is not Unix but GNU!

sed is UNIX from the very beginning, while GNU's Not Unix! ;-)

comment:21 in reply to: ↑ 19 Changed 8 years ago by leif

Replying to tscrim:

Also there is no difference in conflicts with a branch that gets merged into develop versus a script that fixes these things right before release.

Well, in application, there is.

Also, I'd apply it last before preparing a stable release, since developers will have to rebase their pending patches afterwards anyway. (Of course the number of potential conflicts with work in progress they'll face doesn't really change though.)

comment:22 Changed 8 years ago by wluebbe

This ticket now has

  • 177 files changed, 515 insertions, 515 deletions

but ticket:15990 had

  • 503 files changed, 3138 insertions, 3170 deletions

On the way there were some merge conflict, but nothing serious.
Therefore I'm not afraid about conflicts (for this ticket).

comment:23 follow-up: Changed 8 years ago by vdelecroix

  • Status changed from needs_review to needs_work

Hi,

1) As I already told you, the substitutions made bad things to the documentation. Hopefully, in very few places. For examples

  • /src/sage/ext/gen_interpreters.py: we had "so len==None is different than len==1" and now it is "so len is None is different than len==1". The new sentence is much harder to read.
  • /src/sage/coding/code_bounds.py: in a comment we had "algorithm==None or algorithm="gap"" would be better to have "algorithm=None or algorithm="gap" (instead of the is None here).
  • etc

2) You start removing unnecessary parenthesis. First of all it is not the job of that ticket. Secondly, one can arguably says that code is more readable with more parenthesis. So revert all those changes. If you want to remove parenthesis (which I think is a bad idea in general) then open a discussion on sage-devel and then a ticket if a consensus is reached.

Vincent

comment:24 in reply to: ↑ 23 Changed 8 years ago by leif

Replying to vdelecroix:

1) As I already told you, the substitutions made bad things to the documentation. Hopefully, in very few places.

Yep, at least where "is None" isn't typeset as code.

And I also wouldn't touch most of the comments.


2) You start removing unnecessary parenthesis. First of all it is not the job of that ticket. Secondly, one can arguably says that code is more readable with more parenthesis. So revert all those changes. If you want to remove parenthesis (which I think is a bad idea in general) then open a discussion on sage-devel and then a ticket if a consensus is reached.

I'm in general ok with changing the "C style" if (cond): and while (cond): to usual Python syntax (without outer parentheses), but I agree that this should be done on another ticket, especially since currently only some instances (namely those in or near the context of other changes) get "normalized".

There are also a couple of conditions where I'd personally change the order of evaluation, of the form if <expensive_test> <and|or> <cheap_test>:, but that's also out of the scope of this ticket.

comment:25 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:26 Changed 8 years ago by wluebbe

  • Branch changed from u/wluebbe/ticket/16216 to u/wluebbe/ticket/16216b
  • Commit changed from 1219d56b731119122356714ed4d26ae33c63cd5d to 6a2e8067c481abe8b5230dd1be3d86a9d9aa3d85
  • Status changed from needs_work to needs_review

I pushed a fresh start based on Sage 6.2.

From commit message of the manual changes:

    * changed "not xxx is None" to "xxx is not None" (where appropriate)
    * reverted changes in docstring text
      - usually did not revert changes to doctest *code*
      - but reverted changes to expected results (if required)
    * (usually) did not revert changes to *code* in comments
    
    Always tried to apply good judgement - but refrained from changes
    unrelated to the subject "is None".

New commits:

f623de2generated changes from "== None" to "is None"
6a2e806manual fixup of the generated changes

comment:27 Changed 8 years ago by git

  • Commit changed from 6a2e8067c481abe8b5230dd1be3d86a9d9aa3d85 to f87a26fcbfd492b52242c1ccddf62659332c10f0

Branch pushed to git repo; I updated commit sha1. New commits:

f87a26fMerge branch 'develop' into u/wluebbe/ticket/16216b

comment:28 Changed 8 years ago by rws

  • Reviewers set to Ralf Stephan

I've eyeballed it all and patchbot is happy, too. So, if you remove that unrelated change in combinat/backtrack you can set positive.

comment:29 Changed 8 years ago by wluebbe

That unrelated change in combinat/backtrack resulted from resolving the merge conflict (comment 27). So it is not mine ;-) and I am reluctant removing it.

Positive review anyway?

comment:30 Changed 8 years ago by rws

But develop doesn't have this code so you introduced (or re-introduced) it.

comment:31 Changed 8 years ago by rws

If you don't know the best way to check it: git diff develop when in your up-to-date branch should show only your own changes.

comment:32 Changed 8 years ago by git

  • Commit changed from f87a26fcbfd492b52242c1ccddf62659332c10f0 to 564c73c8cab8aef1ec1c9e626a8e280ea8217c8f

Branch pushed to git repo; I updated commit sha1. New commits:

564c73ctrac #16216: undo a mistake from the last merge

comment:33 Changed 8 years ago by wluebbe

  • Status changed from needs_review to positive_review

Thanks Ralf, for the help! The merge error is fixed.

I set to positive as you suggested in comment 28.

comment:34 Changed 8 years ago by vbraun

  • Branch changed from u/wluebbe/ticket/16216b to 564c73c8cab8aef1ec1c9e626a8e280ea8217c8f
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.