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

style: Remove stale references to isort #1631

Merged
merged 1 commit into from
Sep 11, 2024

Conversation

romanc
Copy link
Contributor

@romanc romanc commented Sep 6, 2024

Description

isort has been replaced by ruff in PR #1466.

Note Removing isort means rebuilding the requirements, which updates a couple of unrelated (to the isort removal) dependencies.

Note 2 I also checked for black (formatter) and there are lingering references too. However, black is actively used as formatter in code generation

"""Format Python source code using black formatter."""

Requirements

  • All fixes and/or new features come with corresponding tests.
  • Important design decisions have been documented in the approriate ADR inside the docs/development/ADRs/ folder.

If this PR contains code authored by new contributors please make sure:

  • The PR contains an updated version of the AUTHORS.md file adding the names of all the new contributors.

isort has been replaced by ruff with PR
GridTools#1466.

Removing isort means rebuilding the requirements, which updates a couple
of unrelated (to the isort removal) dependencies.
@romanc romanc changed the title Remove stale reference to isort style: Remove stale references to isort Sep 6, 2024
@romanc
Copy link
Contributor Author

romanc commented Sep 6, 2024

/cc @egparedes I found more leftovers from the ruff refactor. Please review.

Copy link
Contributor

@egparedes egparedes left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -12,7 +12,7 @@
import pytest


pytest.importorskip("atlas4py") # isort: skip
pytest.importorskip("atlas4py")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit surprised this is not longer needed, but it's good to know. Also note that the #isort: skip comments are actually respected by ruff (https://docs.astral.sh/ruff/linter/#action-comments) although it would have been better to add the # ruff: prefix as it is mentioned in the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was also surprised that nothing breaks, but since it works at is, I think it's better to remove them. Good to know (for the future) that ruff respects these.

@egparedes
Copy link
Contributor

cscs-ci run

@romanc
Copy link
Contributor Author

romanc commented Sep 10, 2024

cscs-ci run

Do I read the logs correctly and the two tests just failed because there weren't any nodes responding (to slurm) in the 8h time window? Or do you think it is stuck somewhere in setup (given that I changed dependencies)?

@egparedes
Copy link
Contributor

cscs-ci run

Do I read the logs correctly and the two tests just failed because there weren't any nodes responding (to slurm) in the 8h time window? Or do you think it is stuck somewhere in setup (given that I changed dependencies)?

No, I think you're right and failure is unrelated to the changes in this PR. Let's try again.

@egparedes
Copy link
Contributor

cscs-ci run

@egparedes egparedes merged commit 712324c into GridTools:main Sep 11, 2024
43 checks passed
@romanc romanc deleted the romanc/remove-stale-isort branch September 11, 2024 09:14
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