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

Use ruff format to ensure UNIX line endings in Python files #2770

Merged
merged 7 commits into from
Nov 16, 2023

Conversation

weiji14
Copy link
Member

@weiji14 weiji14 commented Oct 25, 2023

Description of proposed changes

Automatically convert line endings in Python files to \n (UNIX style). Requires using beta version of Ruff formatter in v0.1.2, see https://astral.sh/blog/the-ruff-formatter.

We've actually been using dos2unix to do the line ending checks since #736, but not to do the actual formatting, probably because dos2unix is a little trickier to install on Windows (?). Using ruff though is a little easier since we can install it with pip/conda-forge.

Caveats:

  • This only runs on Python files that ruff looks at, and not non-Python files like the .rst, .yml, .md files

Partially addresses #736 (comment)

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

Automatically convert line endings in Python files to `\n` (UNIX style). Requires using beta version of Ruff formatter in v0.1.2. Also sorting ruff sections in pyproject.toml alphabetically.
@weiji14 weiji14 added the maintenance Boring but important stuff for the core devs label Oct 25, 2023
@weiji14 weiji14 self-assigned this Oct 25, 2023
Some deviations between the black and ruff linters on where to place inline comments.
@weiji14
Copy link
Member Author

weiji14 commented Oct 25, 2023

/format

@weiji14
Copy link
Member Author

weiji14 commented Oct 25, 2023

See example commit at 22b194c triggered by #2770 (comment) where the CRLF line-endings were converted to LF endings.

@weiji14 weiji14 marked this pull request as ready for review October 25, 2023 04:56
@weiji14
Copy link
Member Author

weiji14 commented Oct 25, 2023

There are some warnings about tab-indentation (W191) from running ruff format:

ruff format --check pygmt doc/conf.py examples
warning: The following rules may cause conflicts when used with the formatter: 'W191'. To avoid unexpected behavior, we recommend disabling these rules, either by removing them from the `select` or `extend-select` configuration, or adding then to the `ignore` configuration.
warning: The following rules may cause conflicts when used with the formatter: 'W191'. To avoid unexpected behavior, we recommend disabling these rules, either by removing them from the `select` or `extend-select` configuration, or adding then to the `ignore` configuration.

See also upstream issue at astral-sh/ruff#8185, these warnings might go away with astral-sh/ruff#8196 🤞

@seisman
Copy link
Member

seisman commented Oct 25, 2023

If I understand it correctly, ruff format will replace black, right? If yes, then I think we should split it into two separate RPs:

  1. Use ruff format and remove black
  2. Use ruff to ensure UNIX line endings (unless UNIX line ending is the default)

@weiji14
Copy link
Member Author

weiji14 commented Oct 25, 2023

Ah ok, I was under the impression that ruff format ... was a replacement for ruff check --fix, but apparently not, according to https://docs.astral.sh/ruff/formatter and https://docs.astral.sh/ruff/linter, a formatter is different to a linter. It sounds like we'll need both?

Btw, I could only get the UNIX line formatting to work with ruff format, and not ruff check --fix. I'd maybe add ruff format to this PR (and keep ruff check too), and then deactivate black in a separate PR. Which means we'll have two formatters for a while perhaps.

Edit: It seems like there is some thought towards unifying the linter and formatter in one command - astral-sh/ruff#8232. Gonna mark this as draft first.

@weiji14 weiji14 marked this pull request as draft October 26, 2023 00:09
@seisman
Copy link
Member

seisman commented Nov 16, 2023

Edit: It seems like there is some thought towards unifying the linter and formatter in one command - astral-sh/ruff#8232. Gonna mark this as draft first.

Instead of waiting for the upstream change about linter/formatter, maybe we should go ahead and follow the current ruff documentation and use ruff check/ruff format. We can always update our commands if there are any upstream changes in ruff.

Run the linter `ruff check`, then the formatter `ruff format`.
@weiji14
Copy link
Member Author

weiji14 commented Nov 16, 2023

Edit: It seems like there is some thought towards unifying the linter and formatter in one command - astral-sh/ruff#8232. Gonna mark this as draft first.

Instead of waiting for the upstream change about linter/formatter, maybe we should go ahead and follow the current ruff documentation and use ruff check/ruff format. We can always update our commands if there are any upstream changes in ruff.

Ok, done in 6076a50. Now we run the linter ruff check first, followed by the formatter ruff format. Specifically:

  • make format does ruff check --fix $(FORMAT_FILES) && ruff format $(FORMAT_FILES)
  • make check does ruff check $(FORMAT_FILES) && ruff format --check $(FORMAT_FILES)

@weiji14 weiji14 marked this pull request as ready for review November 16, 2023 04:03
@weiji14 weiji14 added this to the 0.11.0 milestone Nov 16, 2023
@@ -99,14 +99,17 @@ select = [
]
Copy link
Member

Choose a reason for hiding this comment

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

See https://docs.astral.sh/ruff/configuration/.

select should be in the [tool.ruff.lint] section and [tool.ruff.per-file-ignores] should be [tool.ruff.lint.per-file-ignores].

I think it also make sense to sort the section by the following order:

[tool.ruff]

[tool.ruff.lint]

[tool.ruff.lint.per-file-ignores]

[tool.ruff.format]

# tool-specific configurations
[tool.ruff.isort]

[tool.ruff.pycodestyle]

We probably also want to require ruff>=0.1.3.

Copy link
Member Author

@weiji14 weiji14 Nov 16, 2023

Choose a reason for hiding this comment

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

Hmm, did they change the TOML config settings recently? I see pycodestyle is now under tool.ruff.lint.pycodestyle, and isort is under tool.ruff.lint.isort. Let me fix these up.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 8c58089.

@weiji14
Copy link
Member Author

weiji14 commented Nov 16, 2023

If I understand it correctly, ruff format will replace black, right? If yes, then I think we should split it into two separate RPs:

  1. Use ruff format and remove black
  2. Use ruff to ensure UNIX line endings (unless UNIX line ending is the default)

Oh, and just on this, we could remove black in a follow-up PR. I think ruff format does some sort of line-ending formatting by default, so doing 1 and then 2 doesn't make sense.

Most of ruff's lint rules are now under tool.ruff.lint, so rearranging things a bit.
@seisman seisman changed the title CI: Use ruff format to ensure UNIX line endings in python files Use ruff format to ensure UNIX line endings in python files Nov 16, 2023
@seisman
Copy link
Member

seisman commented Nov 16, 2023

Oh, and just on this, we could remove black in a follow-up PR. I think ruff format does some sort of line-ending formatting by default, so doing 1 and then 2 doesn't make sense.

Sounds good to me. I also removed 'CI' from the PR title, because the changes are not directly related to CI.

@seisman seisman changed the title Use ruff format to ensure UNIX line endings in python files Use ruff format to ensure UNIX line endings in Python files Nov 16, 2023
@seisman seisman merged commit b873c36 into main Nov 16, 2023
13 of 16 checks passed
@seisman seisman deleted the ruff/format-line-endings branch November 16, 2023 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Boring but important stuff for the core devs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants