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

Decide on and then enforce line length #214

Open
GiacomoPope opened this issue Sep 2, 2024 · 2 comments
Open

Decide on and then enforce line length #214

GiacomoPope opened this issue Sep 2, 2024 · 2 comments

Comments

@GiacomoPope
Copy link
Contributor

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
Copy link
Collaborator

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: #191 (comment). 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
Copy link
Contributor Author

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.

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

No branches or pull requests

2 participants