flintlib / python-flint

Python bindings for Flint and Arb
MIT License
132 stars 27 forks source link

Decide on and then enforce line length #214

Open GiacomoPope opened 2 months ago

GiacomoPope commented 2 months ago

Not urgent, but making an issue to track this. At some point we probably want to enforce rule E501 for the liner and pick a line length. 120 is pretty long, 80 is standard.

oscarbenjamin commented 2 months ago

I really don't mind. Black's 88 is also a sort of standard in Python land now.

I would like to point out that a lot of this linting stuff started from here: https://github.com/flintlib/python-flint/pull/191#discussion_r1734469381. Note that I was referring to the line length of prose text in a docstring. I think that is a bit of a different case from actual code because the docstring text is shown directly to users e.g.:

In [7]: ? fmpq_poly.factor
Signature:       fmpq_poly.factor(self, *, monic=False)
Call signature:  fmpq_poly.factor(*args, **kwargs)
Type:           cython_function_or_method
String form:    <cyfunction fmpq_poly.factor at 0x72d19c0baec0>
Docstring:     
fmpq_poly.factor(self, *, monic=False)
Factors *self* into irreducible polynomials. Returns (*c*, *factors*)
where *c* is the leading coefficient and *factors* is a list of
(*poly*, *exp*).

    >>> fmpq_poly.legendre_p(5).factor()
    (1/8, [(x, 1), (63*x^4 + (-70)*x^2 + 15, 1)])
    >>> (fmpq_poly([1,-1],10) ** 5 * fmpq_poly([1,2,3],7)).factor()
    (-1/700000, [(3*x^2 + 2*x + 1, 1), (x + (-1), 5)])

Since python-flint 0.7.0 this returns primitive denominator-free
factors consistent with ``fmpq_mpoly.factor()``. In previous versions
of python-flint all factors were made monic. Pass ``monic=True`` to get
monic factors instead.

    >>> fmpq_poly.legendre_p(5).factor(monic=True)
    (63/8, [(x, 1), (x^4 + (-10/9)*x^2 + 5/21, 1)])
    >>> (fmpq_poly([1,-1],10) ** 5 * fmpq_poly([1,2,3],7)).factor(monic=True)
    (-3/700000, [(x^2 + 2/3*x + 1/3, 1), (x + (-1), 5)])

I wrote part of that docstring myself and wrapped all prose parts to 78 characters. One doctest line exceeds 78 characters a little but I decided it was better just to exceed the wrapped line length in that particular case (which I could because there was no linter enforcing any line length limit).

Line length in the code is not user-facing so is different. I do still find it harder to read 120-length lines of code (regardless of how big the monitor is) but I have not complained about many long lines when reviewing many pull requests.

It does seem that line length is a bit tricky in Cython code when calling C functions as well because there are many functions that take many arguments and all function names are always fully qualified and arguments often need casting etc. This often leads to function calls that don't fit in 80 characters but I'm not sure that trying to adhere to some line length would make that code more readable.

GiacomoPope commented 2 months ago

Enforcing 88 length lines comes up with about 500 linting errors, which is annoying but not impossible to hand edit. length 100 is about 250 changes. If we just pick 120 then it's only 35 changes which is essentially "free" to refactor. I don't think there's a way to use any formatting tools to do this for us.

I don't have massive opinions on what we should do, but generally keeping the line lengths shorter makes life easier than harder.

I guess a lot of the long c function calls would end up as:

def some_function(self, other):
    ....
    some_c_binding(
        some, variables, here
    )
    return something

which i think I generally did myself when writing code but probably not everywhere.