search

flintlib / python-flint

Python bindings for Flint and Arb
MIT License
131 stars 25 forks source link

Include ruff #200

Closed GiacomoPope closed 1 month ago

GiacomoPope commented 1 month ago

Currently ruff.toml is the default settings, feel free to suggest changes.

The current output of ruff statistics is:

Jack: python-flint % ruff check --statistics
29  F401    [ ] unused-import
27  F403    [ ] undefined-local-with-import-star
16  E731    [*] lambda-assignment
14  E711    [*] none-comparison
12  E712    [*] true-false-comparison
 9  E721    [ ] type-comparison
 7  E741    [ ] ambiguous-variable-name
 3  F841    [*] unused-variable
 1  E401    [*] multiple-imports-on-one-line

I will try and fix these one at a time in some commits.

GiacomoPope commented 1 month ago

Many of the F401 issues are because of: https://github.com/PyCQA/pyflakes/issues/471 -- I suppose we should decide about whether to follow best practices and define an __all__

GiacomoPope commented 1 month ago

After the above fixes we're at

Jack: python-flint % ruff check --statistics
21  F401    unused-import

Related to the comment above.

oscarbenjamin commented 1 month ago

we should decide about whether to follow best practices and define an __all__

Yes, I think that __all__ should be defined but which files does it want it on?

Is it only the .pyx files or does it also want it in .pxd files?

Also we should remove all import *.

GiacomoPope commented 1 month ago

The __all__ seems to be only needed in the */__init__.py files as far as I can tell. I have also reenabled F403 so we eventually remove the * imports.

Jack: python-flint % ruff check --statistics                     
27  F403    undefined-local-with-import-star
21  F401    unused-import

I will be busy now potentially until the end of the week. Feel free to merge / ignore this if you want and we can continue work whenever.

GiacomoPope commented 1 month ago

actually, i think I have misunderstood ruff, the above has ONLY checked python files.

Jack: python-flint % ruff check src/flint/types/fq_default.pyx --statistics
203     syntax-error

This means I must have missed some flag for the cython files and there might be a LOT more work to do. I will have to look into this more after this week

oscarbenjamin commented 1 month ago

I was a bit surprised that this didn't involve changing 10,000 or so lines...

GiacomoPope commented 1 month ago

we should probably just close this, im not sure this PR really helps with anything currently.

GiacomoPope commented 1 month ago

I also ran

Jack: python-flint % cython-lint src/flint/types/*.pyx
Jack: python-flint % cython-lint --version
0.16.0

on main and got no errors.

oscarbenjamin commented 1 month ago

The actual changes here to the code are mostly not needed or not correct so I guess that the ruff configuration would need to be improved to be useful. I think it could be possible to improve that but it requires choosing which ruff rules to use and configuring them.

Obviously feel free to close if you don't want to work on it.

The cython-lint checks pass now but note that lots of checks are disabled: https://github.com/flintlib/python-flint/blob/be0e41057339b71321d9539558bf054266540cea/pyproject.toml#L42-L45 We would probably not want to ignore all of those but enabling each one would likely then require making some changes somewhere in the codebase.

GiacomoPope commented 1 month ago

OK -- I'll close this for now and if I feel like refactoring I'll look at removing elements from the ignore list and refactoring for each one potentially.