Small changes to please cython-lint

flintlib / python-flint

Python bindings for Flint and Arb

MIT License

131 stars 25 forks source link

Small changes to please cython-lint #211

Closed GiacomoPope closed 1 month ago

GiacomoPope commented 1 month ago

Most changes are trivial.

See issue #210 about one remaining small change. The "last" thing I think we need to decide is a line length so we can fix E501 eventually.

GiacomoPope commented 1 month ago

I don't understand the E202 error in the CI as I don't get this locally and the error itself seems to point to f{x = } in format strings, which should be allowed... seems weird that this lint is failing for the CI machine but not locally on mine?!

GiacomoPope commented 1 month ago

Hmm it seems that people argue about this elsewhere: https://github.com/PyCQA/pycodestyle/issues/1201 and if my E202 doesn't fail but it does on the CI I'll just include it back into the ignore.

oscarbenjamin commented 1 month ago

seems weird that this lint is failing for the CI machine but not locally on mine?!

The two likely explanations for this are always:

Different version of something e.g. different cython-lint or perhaps even different Python version (some lint tools behave differently for different Python versions). Obvious thing is to make sure you have the latest version of cython-lint since that is what CI will use.
The PR branch is not based off of the tip of master i.e. there are further changes on master that are not in the PR branch. In this case a merge/rebase with master should mean that you see the same thing locally as in CI.

oscarbenjamin commented 1 month ago

Hmm it seems that people argue about this elsewhere: PyCQA/pycodestyle#1201

I think what happened there is that everyone agreed it was a bug apart from the pyflakes maintainer who did not want to discuss it and so closed and locked the issue.

Maybe we can do without this rule if it doesn't work properly.

GiacomoPope commented 1 month ago

At the moment the rule is flagging up the format strings (in the CI at least) which I disagree with (I think f"{x = }" produces a better output than f"{x=}"). Let me add some comments to the toml.

oscarbenjamin commented 1 month ago

At the moment the rule is flagging up the format strings (in the CI at least) which I disagree with (I think f"{x = }" produces a better output than f"{x=}")

The question is whether the rule is actually useful in other situations. If it is then we can work around it in this case e.g. f"x = {x}".

If the rule is generally not that important but a bit nice for formatting then I would be most inclined just to "fix" it throughout the codebase and then just leave it excluded in the lint config.

GiacomoPope commented 1 month ago

Looks like E202 is now fixed by doing the (in my opinion worse) option of x = {x} considering {x = } is intended to be used...

Maybe better to do this though than have the lint hack in place, as I don't know if } might come up in other places and be "bad" or whatever...

oscarbenjamin commented 1 month ago

Okay, looks good.