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

Include ruff #200

Closed
wants to merge 10 commits into from
Closed

Include ruff #200

wants to merge 10 commits into from

Conversation

GiacomoPope
Copy link
Contributor

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
Copy link
Contributor Author

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

@GiacomoPope
Copy link
Contributor Author

After the above fixes we're at

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

Related to the comment above.

@oscarbenjamin
Copy link
Collaborator

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
Copy link
Contributor Author

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 GiacomoPope marked this pull request as ready for review August 28, 2024 12:33
@GiacomoPope
Copy link
Contributor Author

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

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

Comment on lines 3671 to +3675
assert (A1 != A2) is False
assert (A1 == B) is False
assert (A1 != B) is True
assert (A1 == None) is False
assert (A1 != None) is True
assert (None == A1) is False
assert (None != A1) is True
assert (A1 is None) is False
assert (A1 is not None) is True
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also the case for some of the other changes but the code here is supposed to be testing __eq__ so it does need to be == rather than is. In cases like this we just need a # ruff: ignore or whatever the flag is.

@GiacomoPope
Copy link
Contributor Author

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

@GiacomoPope
Copy link
Contributor Author

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

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:

[tool.cython-lint]
max-line-length = 120
ignore = ['E128','E129','E202','E221','E222','E261','E262','E265','E501','E731','E741','E743']
exclude = 'src/flint/flintlib/.*'

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
Copy link
Contributor Author

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.

@GiacomoPope GiacomoPope closed this Sep 2, 2024
@GiacomoPope GiacomoPope deleted the ruff_check branch September 2, 2024 15:58
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

Successfully merging this pull request may close these issues.

2 participants