Replace flakeheaven and isort with ruff #2747

weiji14 · 2023-10-15T09:43:43Z

Description of proposed changes

Use ruff to replace both flakeheaven (including pycodestyle and pyflakes rules) and isort, because it is much faster, and will help with Python 3.12 support (xref #2711).

Rulesets we are applying from https://docs.astral.sh/ruff/rules:

Ignoring:

E501 (line-too-long) - a bit complicated, see inline comment below
F401 (unused-import) - only for __init__.py files

TODO

Update the documentation in doc/contributing.md
Update Makefile's format and check commands.
GitHub Actions CI format-command.yml and style_checks.yml now installs ruff instead of flakeheaven/isort
Gitignoring .ruff_cache/ instead of .flakeheaven_cache/
Update files for W505 (doc-line-too-long)

Supersedes #1847, addresses #2741.

Reminders

Run make format and make check to make sure the code follows the style guide.
Add tests for new features or tests that would have caught the bug that you're fixing.
Add new public functions/methods/classes to doc/api/index.rst.
Write detailed docstrings for all functions/methods.
If wrapping a new module, open a 'Wrap new GMT module' issue and submit reasonably-sized PRs.
If adding new functionality, add an example to docstrings or tutorials.
Use underscores (not hyphens) in names of Python files and directories.

Slash Commands

You can write slash commands (/command) in the first line of a comment to perform
specific operations. Supported slash commands are:

/format: automatically format and lint the code
/test-gmt-dev: run full tests on the latest GMT development version

Use ruff to replace both flakeheaven (including pycodestyle and pyflakes rules) and isort. Updated the documentation in `doc/contributing.md` and the Makefile's `format` and `check` commands. GitHub Actions CI format-command and style_checks also install ruff instead of flakeheaven/isort now.

Xref astral-sh/ruff#3972 and https://docs.astral.sh/ruff/configuration/#error-suppression.

weiji14

The handling of line-too-long in ruff is a bit of a mess (see comments below). But I do like the speed of ruff ⚡!

weiji14 · 2023-10-15T10:10:23Z

pyproject.toml

-[tool.flakeheaven]
-max_line_length = 88
-max_doc_length = 79
-show_source = true
+[tool.ruff]
+line-length = 88  # E501 (line-too-long)
+show-source = true
+select = [
+    "E",    # pycodestyle
+    "F",    # pyflakes
+    "I",    # isort
+    "W",    # pycodestyle warnings
+]
+ignore = ["E501"]  # Avoid enforcing line-length violations


Previously with flakeheaven, we set max_line_length=88 (to match black). But setting line-length=88 with ruff without setting ignore = ["E501"] would raise errors on lines like these:

pygmt/helpers/decorators.py:206:89: E501 Line too long (98 > 88 characters) | 204 | "interpolation": r""" 205 | interpolation : str 206 | [**b**\|\ **c**\|\ **l**\|\ **n**][**+a**][**+b**\ *BC*][**+c**][**+t**\ *threshold*]. | ^^^^^^^^^^ E501 207 | Select interpolation mode for grids. You can select the type of 208 | spline used: |

Note that black doesn't format these code lines over 88 characters, but ruff complains about it. The thing is, we recently disabled pylint's line-length checking in #2735, because we assumed that flakeheaven's pycodestyle checker would handle it, but ruff's pycodestyle checker is a little different.

So what should we do in this case?

Can we ignore both E501 (for codes) and W505 (for docs) in ruff and enable pylint's line-length checking (I assume it checks both codes and docs).

For long URLs, we can either add # pylint: disable=line-too-long to the lines or improve the regex for the ignore-long-lines option to make it work for lines like

.. figure: https://very-long-urls.com

enable pylint's line-length checking (I assume it checks both codes and docs).

I'm considering that we replace pylint with ruff eventually (see #2741 (comment)), so would prefer not to add pylint's line-length checking back.

Can we ignore both E501 (for codes) and W505 (for docs) in ruff

E501 is ignored already right now. We could ignore W505 too I suppose, assuming that blackdoc and docformatter is sufficient, and we don't plan to replace both with ruff 🙂

assuming that blackdoc and docformatter is sufficient, and we don't plan to replace both with ruff 🙂

docformatter: astral-sh/ruff#1335
blackdoc: astral-sh/ruff#7146

. We could ignore W505 too I suppose, assuming that blackdoc and docformatter is sufficient, and we don't plan to replace both with ruff

Sounds good to me.

Hmm actually, I just did some tesitng and maybe we shouldn't ignore W505. I just tried ignoring W505 in pyproject.toml and removed all the noqa: W505, and blackdoc/docformatter doesn't format things properly when I run make format:

docformatter --in-place pygmt doc/conf.py examples black pygmt doc/conf.py examples All done! ✨ 🍰 ✨ 277 files left unchanged. blackdoc pygmt doc/conf.py examples All done! ✨ 🍰 ✨ 277 files left unchanged. ruff check --fix pygmt doc/conf.py examples pygmt/datasets/earth_age.py:75:80: W505 Doc line too long (133 > 79 characters) | 73 | (i.e., ``grid.gmt.registration`` and ``grid.gmt.gtype`` respectively). 74 | However, these properties may be lost after specific grid operations (such 75 | as slicing) and will need to be manually set before passing the grid to any PyGMT data processing or plotting functions. Refer to | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ W505 76 | :class:`pygmt.GMTDataArrayAccessor` for detailed explanations and 77 | workarounds. |

https://docs.astral.sh/ruff/rules/doc-line-too-long/#why-is-this-bad

Ignores lines that end with a URL, as long as the URL starts before the line-length threshold.

I think it means that we can simply remove all noqa: W505 and don't have to ignore W505.

https://docs.astral.sh/ruff/rules/doc-line-too-long/#why-is-this-bad

Ignores lines that end with a URL, as long as the URL starts before the line-length threshold.

I think it means that we can simply remove all noqa: W505 and don't have to ignore W505.

Tried removing all the noqa: W505 in a208ca8. Style Checks show these errors:

ruff check pygmt doc/conf.py examples pygmt/datasets/earth_age.py:75:80: W505 Doc line too long (133 > 79 characters) | 73 | (i.e., ``grid.gmt.registration`` and ``grid.gmt.gtype`` respectively). 74 | However, these properties may be lost after specific grid operations (such 75 | as slicing) and will need to be manually set before passing the grid to any PyGMT data processing or plotting functions. Refer to | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ W505 76 | :class:`pygmt.GMTDataArrayAccessor` for detailed explanations and 77 | workarounds. | pygmt/datasets/samples.py:334:80: W505 Doc line too long (80 > 79 characters) | 332 | >>> # use list_sample_data to see the available datasets 333 | >>> pprint(list_sample_data(), width=120) 334 | {'bathymetry': 'Table of ship bathymetric observations off Baja California', | ^ W505 335 | 'earth_relief_holes': 'Regional 20 arc-minutes Earth relief grid with holes', 336 | 'fractures': 'Table of hypothetical fracture lengths and azimuths', | pygmt/datasets/samples.py:335:80: W505 Doc line too long (82 > 79 characters) | 333 | >>> pprint(list_sample_data(), width=120) 334 | {'bathymetry': 'Table of ship bathymetric observations off Baja California', 335 | 'earth_relief_holes': 'Regional 20 arc-minutes Earth relief grid with holes', | ^^^ W505 336 | 'fractures': 'Table of hypothetical fracture lengths and azimuths', 337 | 'hotspots': 'Table of locations, names, and symbol sizes of hotpots from Müller et al. (1993)', | pygmt/datasets/samples.py:337:80: W505 Doc line too long (100 > 79 characters) | 335 | 'earth_relief_holes': 'Regional 20 arc-minutes Earth relief grid with holes', 336 | 'fractures': 'Table of hypothetical fracture lengths and azimuths', 337 | 'hotspots': 'Table of locations, names, and symbol sizes of hotpots from Müller et al. (1993)', | ^^^^^^^^^^^^^^^^^^^^^ W505 338 | 'japan_quakes': 'Table of earthquakes around Japan from the NOAA NGDC database', 339 | 'mars_shape': 'Table of topographic signature of the hemispheric dichotomy of Mars from Smith and Zuber (1996)', | pygmt/datasets/samples.py:338:80: W505 Doc line too long (85 > 79 characters) | 336 | 'fractures': 'Table of hypothetical fracture lengths and azimuths', 337 | 'hotspots': 'Table of locations, names, and symbol sizes of hotpots from Müller et al. (1993)', 338 | 'japan_quakes': 'Table of earthquakes around Japan from the NOAA NGDC database', | ^^^^^^ W505 make: *** [Makefile:72: check] Error 1 339 | 'mars_shape': 'Table of topographic signature of the hemispheric dichotomy of Mars from Smith and Zuber (1996)', 340 | 'maunaloa_co2': 'Table of CO2 readings from Mauna Loa', | pygmt/datasets/samples.py:339:80: W505 Doc line too long (117 > 79 characters) | 337 | 'hotspots': 'Table of locations, names, and symbol sizes of hotpots from Müller et al. (1993)', 338 | 'japan_quakes': 'Table of earthquakes around Japan from the NOAA NGDC database', 339 | 'mars_shape': 'Table of topographic signature of the hemispheric dichotomy of Mars from Smith and Zuber (1996)', | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ W505 340 | 'maunaloa_co2': 'Table of CO2 readings from Mauna Loa', 341 | 'notre_dame_topography': 'Table 5.11 in Davis: Statistics and Data Analysis in Geology', | pygmt/datasets/samples.py:341:80: W505 Doc line too long (93 > 79 characters) | 339 | 'mars_shape': 'Table of topographic signature of the hemispheric dichotomy of Mars from Smith and Zuber (1996)', 340 | 'maunaloa_co2': 'Table of CO2 readings from Mauna Loa', 341 | 'notre_dame_topography': 'Table 5.11 in Davis: Statistics and Data Analysis in Geology', | ^^^^^^^^^^^^^^ W505 342 | 'ocean_ridge_points': 'Table of ocean ridge points for the entire world', 343 | 'rock_compositions': 'Table of rock sample compositions', | pygmt/src/nearneighbor.py:43:80: W505 Doc line too long (80 > 79 characters) | 41 | Grid table data using a "Nearest neighbor" algorithm. 42 | 43 | **nearneighbor** reads arbitrarily located (*x*, *y*, *z*\ [, *w*]) triplets | ^ W505 44 | [quadruplets] and uses a nearest neighbor algorithm to assign a weighted 45 | average value to each node that has one or more data points within a search | pygmt/src/nearneighbor.py:142:80: W505 Doc line too long (83 > 79 characters) | 140 | >>> data = pygmt.datasets.load_sample_data(name="bathymetry") 141 | >>> # Create a new grid with 5 arc-minutes spacing in the designated region 142 | >>> # Set search_radius to only consider points within 10 arc-minutes of a node | ^^^^ W505 143 | >>> output = pygmt.nearneighbor( 144 | ... data=data, | Found 9 errors.

You're right that the long URLs from #2728 don't raise W505 errors anymore, but there are 9 others we'll need to fix.

weiji14 · 2023-10-15T10:15:21Z

pygmt/datasets/earth_age.py

@@ -91,7 +91,7 @@ def load_earth_age(resolution="01d", region=None, registration=None):
    ...     region=[120, 160, 30, 60],
    ...     registration="gridline",
    ... )
-    """
+    """  # noqa: W505


In commit ece0730, I had to move these noqa: W505 (doc-too-long) comments (mostly added in #2728) to behind the triple quotes instead of inline, because ruff requires it to be like so. See astral-sh/ruff#3972 and https://docs.astral.sh/ruff/configuration/#error-suppression.

seisman · 2023-10-15T10:17:48Z

In doc/conf.py, better to change # isort: on to # ruff: isort: on. See https://docs.astral.sh/ruff/configuration/#action-comments

Xref https://docs.astral.sh/ruff/configuration/#action-comments Co-Authored-By: Dongdong Tian <[email protected]>

weiji14 · 2023-10-15T10:27:44Z

pyproject.toml

-[tool.isort]
-profile = "black"
-skip_gitignore = true


Note that ruff's isort rules is nearly-equivalent to isort's profile="black" settings (see https://docs.astral.sh/ruff/faq/#how-does-ruffs-import-sorting-compare-to-isort). Also, files that are gitignored are automatically skipped from being check (see astral-sh/ruff#1234), so no need to apply these rules anymore.

pygmt/datasets/samples.py

Add noqa: W505 back to pygmt/datasets/samples.py, and trim lines in nearneighbor.py.

weiji14 · 2023-10-15T12:40:07Z

Ok, Style Checks all pass now. I'll be busy at a conference for the rest of the week, and not sure if I have time to make substantial changes, so feel free to merge this (or push changes to this branch) once everything looks good.

weiji14 added the maintenance Boring but important stuff for the core devs label Oct 15, 2023

weiji14 added this to the 0.11.0 milestone Oct 15, 2023

weiji14 self-assigned this Oct 15, 2023

weiji14 mentioned this pull request Oct 15, 2023

Use ruff to improve code quality #2741

Closed

Fix W505 line-too-long lint error by moving noqa after the triple quotes

ece0730

Xref astral-sh/ruff#3972 and https://docs.astral.sh/ruff/configuration/#error-suppression.

weiji14 commented Oct 15, 2023

View reviewed changes

weiji14 added the needs review This PR has higher priority and needs review. label Oct 15, 2023

Use ruff: isort: on instead of isort: on

03e0357

Xref https://docs.astral.sh/ruff/configuration/#action-comments Co-Authored-By: Dongdong Tian <[email protected]>

weiji14 commented Oct 15, 2023

View reviewed changes

Try removing all noqa: W505

a208ca8

seisman reviewed Oct 15, 2023

View reviewed changes

pygmt/datasets/samples.py Show resolved Hide resolved

Fix new W505 line-too-long errors

b369811

Add noqa: W505 back to pygmt/datasets/samples.py, and trim lines in nearneighbor.py.

weiji14 marked this pull request as ready for review October 15, 2023 12:37

seisman approved these changes Oct 15, 2023

View reviewed changes

seisman added final review call This PR requires final review and approval from a second reviewer and removed needs review This PR has higher priority and needs review. labels Oct 15, 2023

Merge branch 'main' into ruff

ead3abe

seisman removed the final review call This PR requires final review and approval from a second reviewer label Oct 15, 2023

seisman merged commit 224d1ef into main Oct 15, 2023
13 checks passed

seisman deleted the ruff branch October 15, 2023 16:49

weiji14 mentioned this pull request Oct 15, 2023

Add support for Python 3.12 #2711

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace flakeheaven and isort with ruff #2747

Replace flakeheaven and isort with ruff #2747

weiji14 commented Oct 15, 2023 •

edited

Loading

weiji14 left a comment

weiji14 Oct 15, 2023

seisman Oct 15, 2023

weiji14 Oct 15, 2023

seisman Oct 15, 2023

seisman Oct 15, 2023

weiji14 Oct 15, 2023

seisman Oct 15, 2023

weiji14 Oct 15, 2023

weiji14 Oct 15, 2023

seisman commented Oct 15, 2023

weiji14 Oct 15, 2023

weiji14 commented Oct 15, 2023

Replace flakeheaven and isort with ruff #2747

Replace flakeheaven and isort with ruff #2747

Conversation

weiji14 commented Oct 15, 2023 • edited Loading

weiji14 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seisman commented Oct 15, 2023

Choose a reason for hiding this comment

weiji14 commented Oct 15, 2023

weiji14 commented Oct 15, 2023 •

edited

Loading