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 for linting #289

Merged
merged 10 commits into from
Dec 2, 2024
Merged

Use Ruff for linting #289

merged 10 commits into from
Dec 2, 2024

Conversation

unkcpz
Copy link
Member

@unkcpz unkcpz commented Nov 27, 2024

The ruff pre-commit configs are mainly get from aiida-core.
For the linter complain in tests, I simply add # noqa to silent those.

  • rename test folder to tests

@unkcpz unkcpz marked this pull request as draft November 27, 2024 14:50
@unkcpz unkcpz marked this pull request as ready for review November 27, 2024 15:12
@unkcpz unkcpz marked this pull request as draft November 27, 2024 15:16
@unkcpz unkcpz force-pushed the ruff branch 5 times, most recently from 32ecbe2 to 4150816 Compare November 27, 2024 17:44
@unkcpz unkcpz marked this pull request as ready for review November 27, 2024 17:48
@unkcpz unkcpz requested a review from sphuber November 27, 2024 17:48
pyproject.toml Outdated Show resolved Hide resolved
Comment on lines +596 to +597
# XXX: can return a reference and gracefully use task to cancel itself when the upper call stack fails
asyncio.ensure_future(proc.step_until_terminated()) # noqa: RUF006
Copy link
Member Author

Choose a reason for hiding this comment

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

The linter complain is the reference to the coroutine is not returned. I think the standard way is to use the task to do graceful shutdown.
I'll further investigate on it, what I think is the returned reference to the task returned to the function where it is called (ProcessLauncher) and before this task get run it can be cancelled without other exception from the event loop.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me know if you think it is better to not put dev notes in the src code. @agoscinski

Choose a reason for hiding this comment

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

Should be okay but I dont fully understand this comment What does the XXX mean? Shouldn't ensure_future return a Future or Task? What do you mean with "reference"?

Copy link
Member Author

Choose a reason for hiding this comment

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

According to https://docs.python.org/3/library/asyncio-future.html#asyncio.ensure_future, the API is deprecated and using create_task is in favor. It returns a task, and task is a future like object that can be resolved by running at event loop.

The reference means the task reference as the handler that can pass around to control this created task. At the moment, we did nothing to the task if other issues happened in the event loop. One example in my mind was when the proc.step_until_terminated() is scheduled in the event loop but not yet run, the event loop needs to be shutdown and the handler here need to call to cancel the task in the event loop first.

Choose a reason for hiding this comment

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

ok and the XXX?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don’t remember where I got this habit from but there are some archaeology post to read about this https://www.snellman.net/blog/archive/2017-04-17-xxx-fixme/

I think for modern ide it is highlighted as TODO nd FIXME.

test/test_expose.py Show resolved Hide resolved
@unkcpz
Copy link
Member Author

unkcpz commented Dec 1, 2024

I move the fix for passing 3.12 tests to #292 so this PR only contains the changes recommended from ruff. It is ready to be review.

Copy link
Collaborator

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Thanks @unkcpz

@sphuber sphuber merged commit 55e05e9 into aiidateam:master Dec 2, 2024
7 checks passed
unkcpz added a commit to unkcpz/plumpy that referenced this pull request Dec 14, 2024
* Bump version for pytest and pytest tools
* Use ruff with aligning to aiida-core ruff config
* Update mypy to 1.13.0
* Remove all pylint ignore annotations
* Bump ruff to 0.8.0 for sort the imports
* Update python version to 3.12 for pre-commit CI
* Double quotes for eval inside single quotes f-strings
* Exclude tests/.* for ruff linting
* Do not fail-fast so tests for different python version independent
khsrali pushed a commit that referenced this pull request Dec 19, 2024
* Bump version for pytest and pytest tools
* Use ruff with aligning to aiida-core ruff config
* Update mypy to 1.13.0
* Remove all pylint ignore annotations
* Bump ruff to 0.8.0 for sort the imports
* Update python version to 3.12 for pre-commit CI
* Double quotes for eval inside single quotes f-strings
* Exclude tests/.* for ruff linting
* Do not fail-fast so tests for different python version independent
khsrali pushed a commit that referenced this pull request Dec 19, 2024
* Bump version for pytest and pytest tools
* Use ruff with aligning to aiida-core ruff config
* Update mypy to 1.13.0
* Remove all pylint ignore annotations
* Bump ruff to 0.8.0 for sort the imports
* Update python version to 3.12 for pre-commit CI
* Double quotes for eval inside single quotes f-strings
* Exclude tests/.* for ruff linting
* Do not fail-fast so tests for different python version independent

(cherry picked from commit 55e05e9)
khsrali pushed a commit that referenced this pull request Dec 19, 2024
* Bump version for pytest and pytest tools
* Use ruff with aligning to aiida-core ruff config
* Update mypy to 1.13.0
* Remove all pylint ignore annotations
* Bump ruff to 0.8.0 for sort the imports
* Update python version to 3.12 for pre-commit CI
* Double quotes for eval inside single quotes f-strings
* Exclude tests/.* for ruff linting
* Do not fail-fast so tests for different python version independent

(cherry picked from commit 55e05e9)
khsrali pushed a commit that referenced this pull request Dec 19, 2024
* Bump version for pytest and pytest tools
* Use ruff with aligning to aiida-core ruff config
* Update mypy to 1.13.0
* Remove all pylint ignore annotations
* Bump ruff to 0.8.0 for sort the imports
* Update python version to 3.12 for pre-commit CI
* Double quotes for eval inside single quotes f-strings
* Exclude tests/.* for ruff linting
* Do not fail-fast so tests for different python version independent

(cherry picked from commit 55e05e9)
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.

3 participants