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.

Sign up for GitHub

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

Jump to bottom

[POC] linting with ruff #550

Open
5 tasks
adebardo opened this issue Jul 31, 2024 · 8 comments · May be fixed by #674
Open
5 tasks

[POC] linting with ruff #550

adebardo opened this issue Jul 31, 2024 · 8 comments · May be fixed by #674
Labels
[POC] Conception To review Tickets needs approval about it conception

Comments

@adebardo
Copy link

adebardo commented Jul 31, 2024 •
edited
Loading

Context

The goal of this ticket is to implement linting on the code. We have noticed that certain rules applied to demcompare are not applied in the test files of xDEM.

Examples of errors present in the test files:

  • tests/test_volume.py:252:11: W0125: Using a conditional statement with a constant value (using-constant-test)
  • tests/test_spatialstats.py:563:12: W0612: Unused variable 'ny' (unused-variable)
  • tests/test_spatialstats.py:509:0: C0115: Missing class docstring (missing-class-docstring)
  • tests/test_spatialstats.py:547:4: R0914: Too many local variables (27/15) (too-many-locals)
  • tests/test_vcrs.py:22:8: C0206: Consider iterating with .items() (consider-using-dict-items)

We cannot guarantee that the same code quality is applied to all the code.

Ruff

We propose using the Ruff linter to reduce the number of tools used in xdem.

  • Add the .ruff.toml file
# Exclude a variety of commonly ignored directories.  
exclude = [  
    ".bzr",  
    ".direnv",  
    ".eggs",  
    ".git",  
    ".git-rewrite",  
    ".hg",  
    ".ipynb_checkpoints",  
    ".mypy_cache",  
    ".nox",  
    ".pants.d",  
    ".pyenv",  
    ".pytest_cache",  
    ".pytype",  
    ".ruff_cache",  
    ".svn",  
    ".tox",  
    ".venv",  
    ".vscode",  
    "__pypackages__",  
    "_build",  
    "buck-out",  
    "build",  
    "dist",  
    "node_modules",  
    "site-packages",  
    "venv",  
]  
  
# Same as Black.  
line-length = 120  
indent-width = 4  
  
# Assume Python 3.8  
target-version = "py38"  
  
[lint]  
# Enable Pyflakes (`F`) and a subset of the pycodestyle (`E`)  codes by default.  
# Unlike Flake8, Ruff doesn't enable pycodestyle warnings (`W`) or  
# McCabe complexity (`C901`) by default.  
select = ["ALL"]  
ignore = []  
  
# Allow fix for all enabled rules (when `--fix`) is provided.  
fixable = ["ALL"]  
unfixable = []  
  
# Allow unused variables when underscore-prefixed.  
dummy-variable-rgx = "^(_+|(_+[a-zA-Z0-9_]*[a-zA-Z0-9]+?))$"  
  
[format]  
# Like Black, use double quotes for strings.  
quote-style = "double"  
  
# Like Black, indent with spaces, rather than tabs.  
indent-style = "space"  
  
# Like Black, respect magic trailing commas.  
skip-magic-trailing-comma = false  
  
# Like Black, automatically detect the appropriate line ending.  
line-ending = "auto"  
  
# Enable auto-formatting of code examples in docstrings. Markdown,  
# reStructuredText code/literal blocks and doctests are all supported.  
#  
# This is currently disabled by default, but it is planned for this  
# to be opt-out in the future.  
docstring-code-format = false  
  
# Set the line length limit used when formatting code snippets in  
# docstrings.  
#  
# This only has an effect when the `docstring-code-format` setting is  
# enabled.  
docstring-code-line-length = "dynamic"
  • add ruff in makefile
lint: ruff check $(SRC_DIRS)
  • add ruf in CI
name: Run 
	Ruff run: | 
		ruff check .
  • add ruff in setup.cfg

Correction

  • Ruff return lots of problem that needed to be fixed

Estimate

3d
@adebardo adebardo added the [POC] Conception To review Tickets needs approval about it conception label Jul 31, 2024
@adebardo
Copy link
Author

@adehecq , @duboise-cnes tell me if you agreed

@duboise-cnes
Copy link
Member

on the general linting, i agree to put something at the beginning to benefit from the quality wrappers for all code to be done afterwards. I think it is a must have with this too permissive python language. I would nevertheless prefer go to :

  • quicker ruff directly for replacement of pylint (not all but i think sufficient and growing), black, flake8, isort. only one tool easier
  • and mypy very important also (type checking is really important). Maybe another ticket.

See https://docs.astral.sh/ruff/faq/ and ruff is now really mature and faster. So it would be a better choice on linting i would advise

"Explain why pylint is in the "test" dependencies in the setup file and see if it's the most appropriate place for it" --> you mean use a "dev" target ? more explicit but if xdem is used with test for dev parts, go for it. No casus belli ;)

max line to 120 is a lot for my old me used of little screen but it is the dev standard so ok for me if the code is already there ;)

max-module-lines is also important for refactoring goal. 700 or 1000 max could be another issue in parallel of code structure possible refactoring

@adebardo
Copy link
Author

adebardo commented Aug 1, 2024 •
edited
Loading

Hello, I am glad you suggested using ruff. I will revise the ticket with this solution.
And an think about a refactoring for smaller files

@adebardo adebardo added [POC] Conception stuck The issue conception is stopped and removed [POC] Conception To review Tickets needs approval about it conception labels Aug 1, 2024
@adebardo
Copy link
Author

adebardo commented Aug 1, 2024 •
edited
Loading

Ruff seems bad as it job :

with ruff :
image

with pylint:
image

@duboise-cnes
Copy link
Member

Ok see internally if it is misconfiguration of ruff and if not enough mature (it's weird nonetheless) I think we keep isort, black, flake8, pylint, mypy as a baseline.

@adehecq
Copy link
Member

adehecq commented Aug 9, 2024

I didn't know riff, but I think if we could reduce the number of linters to one that manages all, that would certainly make things easier.
It's seems normal that ruff does not complain in some parts while pylint does. As stated in the FAQ, ruff and pylint do not completely overlap on rules. Maybe this can be tuned a bit with configuration, but otherwise we just have to accept that no tool will perfectly catch all issues.

As for the line length, 79 is really too short when there are 2, 3, 4 indentation levels... 120 is a bit more comfortable.
For the maximum number of lines, let's try to not be too strict for the moment, as we have rather long files in geoutils/xdem at the moment 😅

@rhugonnet
Copy link
Member

Agree on all.
For ruff, I think @erikmannerfelt mentioned it about a year ago. At that time the main improvement seemed to be speed for very large projects that had to wait several minutes to perform pre-commit. This is not a problem on this repo yet, but it would be more durable to move to ruff anyways for sure, so all for it as well 🙂

@adebardo
Copy link
Author

I finally found a great config file for Ruff, and it seems suitable for our project. I didn’t ignore any rules, so we can discuss them during development if needed.

P.S.: We can also choose the line length.

@adebardo adebardo changed the title [POC] linting with pylint [POC] linting with ruff Aug 29, 2024
@adebardo adebardo added [POC] Conception To review Tickets needs approval about it conception and removed [POC] Conception stuck The issue conception is stopped labels Aug 29, 2024
@duboise-cnes duboise-cnes mentioned this issue Aug 30, 2024
Open
8 tasks
@quinto-clara quinto-clara linked a pull request Dec 20, 2024 that will close this issue
Open
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
[POC] Conception To review Tickets needs approval about it conception
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[WIP] Inplement linting with Ruff quinto-clara/xdem
4 participants
@adehecq @rhugonnet @duboise-cnes @adebardo