[WIP] Implement linting with Ruff by quinto-clara · Pull Request #674 · GlacioHack/xdem

quinto-clara · 2024-12-20T09:26:00Z

Resolves #550

Description :

The Pull Request implements linting on xDEM in order to guarantee the same code quality as demcompare.
The idea was to limit the number of tools and to apply the same rules as demcompare.
That's why we choose to implement Ruff in Xdem, a fast python linter and code formatter gathering a lot of functionalities and replacing pylint, black, flake8 and isort.

Key changes :

Ruff configuration :
- ruff.toml file addded including the following rules :
  - Target version of python : 3.10
  - The formatter will wraps lines at a length of 120 and use 4 spaces per tabulation when a line exceed 120 characters.
  - Ruff will ignore 46 lint rules (for now)
- Ruff added in makefile
- Ruff added in CI file
- Ruff added in setup.cfg file
Code correction :

Ruff returned a lot of errors related to 108 lint rules :
- 62 of them has been fixed, the remaining is ignored and can be fixed later

vschaffn · 2024-12-23T10:13:12Z

In the tests, the protected attributes (like _need_vars) were changed to non-protected attributes (for example, need_vars). This is causing issues because the non-protected attributes may not actually exist in the class in the same form, which leads to errors. Protected attributes (with a leading underscore) are intentionally kept as internal members of the class and might not have corresponding non-protected versions.

It seems that these changes could have been made in response to warnings from Ruff or due to the use of the --fix option. However, the removal of the leading underscore breaks the logic, since the non-protected version of these attributes may not exist.

To fix this:

The protected attributes should be restored with their leading underscores.
We may also want to review the Ruff configuration to ensure it's not suggesting incorrect automatic changes.

vschaffn · 2024-12-23T13:46:25Z

To apply ruff in pre-commits and avoid conflicts with other linters, you would need to add ruff to .pre-commit-config.yaml and remove isort, pylint, flake8 and black.

vschaffn · 2024-12-23T09:14:50Z

Makefile

 ############### GLOBAL VARIABLES ######################
 .DEFAULT_GOAL := help
 SHELL := /bin/bash
+PATH :=.


This command breaks the Makefile by overwriting the system's executable search paths, preventing essential commands from being found. Since PATH already exists, it's unnecessary to redefine it.

vschaffn · 2024-12-23T09:26:14Z

Makefile

+	@echo "Applying ruff..."
+	@echo "================"
+	@echo
+	@ruff --fix $(path)


check --fix $(PATH)

vschaffn · 2024-12-23T09:45:08Z

xdem/ddem.py



-class dDEM(Raster):  # type: ignore
+class Ddem(Raster):  # type: ignore


Not sure this change is very wise, as dDEM mirrors the typo of the DEM class. But if you change it, you have to change it everywhere it is used

vschaffn · 2024-12-23T09:48:58Z

xdem/_typing.py


 # Only for Python >= 3.9
-if sys.version_info.minor >= 9:
+if sys.version_info.minor >= (3, 9):


This causes an error, the system version should not be a tuple

vschaffn · 2024-12-23T10:35:07Z

xdem/coreg/base.py

            steps = list(self.procstep.pipeline)
        argspec = [inspect.getfullargspec(s.__class__) for s in steps]
-        sub_meta = [s._meta["inputs"]["random"]["subsample"] for s in steps]
+        sub_meta = [s.meta["inputs"]["random"]["subsample"] for s in steps]


vschaffn · 2024-12-23T14:36:39Z

xdem/spatialstats.py

    # Get the arrays for proxy values and explanatory variables
    list_all_arr, gsd = _preprocess_values_with_mask_to_array(
-        values=[dvalues] + list_var, include_mask=stable_mask, exclude_mask=unstable_mask, preserve_shape=False
+        values=[dvalues, list_var], include_mask=stable_mask, exclude_mask=unstable_mask, preserve_shape=False,


This modifications triggered an issue : it creates a nested list where list_var is treated as a single element, resulting in [dvalues, [list_var]].

vschaffn · 2024-12-23T14:38:42Z

xdem/spatialstats.py

        list_inside_radius = []
        list_outside_radius: list[float | None] = []
-        binned_ranges = [0.0] + pdist_multi_ranges
+        binned_ranges = [0.0, pdist_multi_ranges]


vschaffn · 2024-12-23T14:38:55Z

xdem/spatialstats.py

            else:
                first_xmin = 0
-            xscale_range_split = [first_xmin] + xscale_range_split
+            xscale_range_split = [first_xmin, xscale_range_split]


vschaffn · 2024-12-23T14:39:08Z

xdem/spatialstats.py


        # Plot the histogram manually with fill_between
-        interval_var = [0] + list(df.lags)
+        interval_var = [0, list(df.lags)]


vschaffn · 2024-12-23T14:39:27Z

xdem/spatialstats.py


        # Get the lags bin centers
-        bins_center = np.subtract(df.lags, np.diff([0] + df.lags.tolist()) / 2)
+        bins_center = np.subtract(df.lags, np.diff([0, df.lags.tolist()]) / 2)


rhugonnet · 2025-01-21T02:16:05Z

Great! 🤩
Tell me if you need more feedback on this PR :)

Question: I heard that Ruff was a "drop-in replacement" for black, flake8, etc (so producing almost exactly the same output, just much much faster). So most of the changes should arise mostly from updating black and other to more recent versions, or different rules, right?
(Out of curiosity, did you try to mirror the current configuration to check that there were almost no changes?)

roelofvandijkO · 2025-06-11T08:26:45Z

.pre-commit-config.yaml

-                  files: ^(xdem|tests)
-
-        # Lint the code using mypy
-        - repo: https://github.com/pre-commit/mirrors-mypy


Ruff does not replace mypy

roelofvandijkO · 2025-06-11T08:29:36Z

This PR would become much easier to merge if you disable most ruff rules and re-enable them one by one.

feat: ruff configuration + errors correction

bbf0ecb

vschaffn reviewed Dec 23, 2024

View reviewed changes

fix: suggestions from the review

0a8ed5f

rhugonnet changed the title ~~[WIP] Inplement linting with Ruff~~ [WIP] Implement linting with Ruff May 6, 2025

roelofvandijkO reviewed Jun 11, 2025

View reviewed changes



		class dDEM(Raster): # type: ignore
		class Ddem(Raster): # type: ignore

Conversation

quinto-clara commented Dec 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description :

Key changes :

Uh oh!

vschaffn commented Dec 23, 2024

Uh oh!

vschaffn commented Dec 23, 2024

Uh oh!

vschaffn Dec 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vschaffn Dec 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vschaffn Dec 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhugonnet commented Jan 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roelofvandijkO commented Jun 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

quinto-clara commented Dec 20, 2024 •

edited

Loading

vschaffn Dec 23, 2024 •

edited

Loading

vschaffn Dec 23, 2024 •

edited

Loading

vschaffn Dec 23, 2024 •

edited

Loading