Pep8-compliance, lattices.py as an example

[jm58660]

This patch nominally tries to pep8fy lattices.py. The real question is discussion about pep8.

CC: @fchapoton

Component: combinatorics

Author: Jori Mäntysalo

Branch/Commit: 8199a58

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/24076

[jm58660]

Branch: u/jmantysalo/pep8-lattices

[jm58660]

comment:2

Frédéric, you might be interested in this.

What should we do to

if ((self.is_ranked() and len(self.meet_irreducibles()) == self.rank()) or
    self.cardinality() == 0):
    return (True, None) if certificate else True

I admit that it is not beatiful, pep8 got it right with E129.

Another thing, is M[i + 1, j] really more readabe than M[i+1, j]? There are also some other places where pep8fying things does not to seem help readability.

[jm58660]

Changed branch from u/jmantysalo/pep8-lattices to none

[jm58660]

Branch: u/jmantysalo/pep8-lattices

[tscrim]

comment:4

Replying to @jm58660:

What should we do to

if ((self.is_ranked() and len(self.meet_irreducibles()) == self.rank()) or
    self.cardinality() == 0):
    return (True, None) if certificate else True

IMO, I wouldn't change this.

Another thing, is M[i + 1, j] really more readabe than M[i+1, j]? There are also some other places where pep8fying things does not to seem help readability.

IMO, we should not be too strictly PEP8, which IIRC allows for breaking things for readability.

Also, while we are at it:

-if isinstance(data, FiniteMeetSemilattice) and len(args) == 0 and len(options) == 0:
+if isinstance(data, FiniteMeetSemilattice) and not args and not options:

[tscrim]

Commit: 2324438

[sagetrac-git]

Changed commit from 2324438 to 5d45753

[sagetrac-git]

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

5d45753

Merge branch 'pep8-lattices2' into t/24076/pep8-lattices

[sagetrac-git]

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

8199a58

"if len(foo) == 0" to "if not foo".

[sagetrac-git]

Changed commit from 5d45753 to 8199a58

[jm58660]

comment:7

I changed those len(foo) == 0 -lines. (And merged to new beta, as I have a compilation problem.)

Now we have following pep warnings:

185 E501 line too long 
  6 E128 continuation line under-indented for visual indent
  4 E129 visually indented line with same indent as next logical line
  2 E731 do not assign a lambda expression, use a def
  2 E202 whitespace before ')'
  2 E201 whitespace after '('

Long lines are mostly impossible to avoid. As for E201/E202 I think it adds readability to use spaces in a long line where outer parentheses are used for line continuation, like

if ( (foor+bar)*zyzzy > 42 and
     some_value*2 < 101 ):

What about lambda expressions? Is there a real reason to use def instead of them? ...actually, why I would use Python in first place if I don't use lambdas...

[tscrim]

comment:8

Replying to @jm58660:

Long lines are mostly impossible to avoid.

Yep, we just do what we can (and for readability of code, somethimes it is better, IMO, to break the limit).

As for E201/E202 I think it adds readability to use spaces in a long line where outer parentheses are used for line continuation, like

if ( (foor+bar)*zyzzy > 42 and
     some_value*2 < 101 ):

I agree; leave them be.

What about lambda expressions? Is there a real reason to use def instead of them? ...actually, why I would use Python in first place if I don't use lambdas...

Well, a lambda function should be anonymous, so you should not assign it a (variable) name. It is mostly equivalent to the def function, but the def is a little more clear about itself and what things do. However, lambdas still serve a purpose for things like map or a key for sort because you can define the little functions that are not associated with a specific name but still need to be functions.

[jm58660]

comment:9

Replying to @tscrim:

What about lambda expressions? Is there a real reason to use def instead of them? ...actually, why I would use Python in first place if I don't use lambdas...

Well, a lambda function should be anonymous, so you should not assign it a (variable) name.

Ah, that was what pep8 complains. First of all, there may be places where a lambda is used more than once. This is not case here. But still, to me

go_up = lambda v: [v_ for v_ in H.neighbors_out(v) if v_ not in above_a]
for v in H.depth_first_search(e, neighbors=go_up):

seems easier than

for v in H.depth_first_search(e, neighbors=lambda v: [v_ for v_ in H.neighbors_out(v) if v_ not in above_a]):

Hence I mark this as needs_review.

For the future it could be nice to pep8-check modified lines, i.e. combine the output with the output of git diff develop.

[sagetrac-git]

Changed commit from 8199a58 to e559cba

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

e559cba

Bikeshedding.

[jm58660]

New commits:

e559cba

Bikeshedding.

[jm58660]

comment:12

Arghs, errors with git...

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

2324438	Trying to be pep8-compliant.
5d45753	Merge branch 'pep8-lattices2' into t/24076/pep8-lattices
8199a58	"if len(foo) == 0" to "if not foo".

[sagetrac-git]

Changed commit from e559cba to 8199a58

[jm58660]

comment:14

Replying to @jm58660:

Arghs, errors with git...

Resolved, hopefully...

[novoselt]

comment:15

I'd like to clarify the purpose of this ticket: is it just an investigation of how pep8 should be applied to "real Sage code" so that we can put appropriate guidelines into the developers guide? That's fine, but I am very much against large scale change of code just for the sake of following guidelines - it does make it harder to track down where certain lines of code are coming from using blame.

Using some script to check NEW lines of code in patches to make sure that future additions/modifications are following good practices is of course a different (and nice) thing.

[tscrim]

comment:16

That's a good point about git blame that I haven't thought about. However, I have found it takes more to convince someone new to Sage that they should follow PEP8 if part of the rest of the file does not, in part because PEP8 says follow the local format. It also makes it easier to maintain the existing code because consistent style is easier to read and parse. So I think these sorts of changes have a net positive and giving the positive review.

[tscrim]

Reviewer: Travis Scrimshaw

[jm58660]

comment:17

Currently we have "Follow the standard Python formatting rules when writing code for Sage - - pep-0008" in the devel manual. From this experience I would say that at most we should add something to soften the tone. ", if there are no special reason to do otherwise." or similar. Maybe nothing.

git blame is hard to use in any case - Frédéric has already touched an enormous number of lines when doing modification for Python3 and has made many typo corrections in the same time.

More general: Should we have more visible way to automatically say "Hey! Please check this!" to a developer from trac? We can not reject the code for being not-pep8-compliant, but we could ask for confirmation.

[vbraun]

Changed branch from u/jmantysalo/pep8-lattices to 8199a58