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

Slightly less ruff #526

Merged
merged 15 commits into from
Jan 2, 2025
Merged

Slightly less ruff #526

merged 15 commits into from
Jan 2, 2025

Conversation

liamhuber
Copy link
Member

I got stuck in authentification hell, so I only had time to just get started; more to come.

Timing test:

from pyiron_workflow import Workflow

%%timeit
Workflow.create.transformer

With lru cache: 45.3 ns ± 0.584 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)

Without: 46.6 ns ± 0.398 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)

A completely inconsequential difference, so I got rid of the creator/wrapper caches. I'll check the remaining cache next.

Copy link

Binder 👈 Launch a binder notebook on branch pyiron/pyiron_workflow/not_as_ruff

Copy link

codacy-production bot commented Dec 20, 2024

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
Report missing for 829daab1 100.00%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (829daab) Report Missing Report Missing Report Missing
Head commit (1539ece) 3356 3071 91.51%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#526) 3 3 100.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more

Footnotes

  1. Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct.

Signed-off-by: liamhuber <[email protected]>
I personally like to use it as an indicator to devs that there's no intention of actually instantiating this class itself, but ruff insists that they are _only_ for specifying interfaces, and without and abstractmethod this doesn't make the cut. I acquiesce.

Signed-off-by: liamhuber <[email protected]>
The operation is simple enough that the savings must have been minimal, and this is not used repeatedly (only when parsing class definitions to start with, e.g. at initial import), so any minor cost is going to be tolerable anyhow.

Signed-off-by: liamhuber <[email protected]>
Per ruff deprecation warning

Signed-off-by: liamhuber <[email protected]>
It was placed intentionally separate from the other imports to distinguish the pyironic single-import user interface from the more standard user/dev public API imports. The line is annotated to prevent ruff from moving it back later.

Signed-off-by: liamhuber <[email protected]>
…o accept the drop in readability from ruff's formatting here. On a quick search, I didn't find any way to modify just this element of the formatting.
Until I either cave on the multiline string formatting, or ruff let's us change it. In the meantime we use black anyhow for formatting.

Signed-off-by: liamhuber <[email protected]>
@liamhuber
Copy link
Member Author

@XzzX, with these amendments I'm happy with #520. Basically I accepted most of the formatting, but ultimately I turned it off and we'll lean on black for now. Otherwise the only anti-ruff thing I did was keep the top-level Workflow import un-sorted, and tell it to always ignore unused imports in __init__.py files (since we use these to specify APIs).

@liamhuber liamhuber marked this pull request as ready for review December 21, 2024 01:01
@liamhuber liamhuber requested a review from XzzX December 21, 2024 01:01
@liamhuber liamhuber mentioned this pull request Dec 21, 2024
Comment on lines +88 to +89
[tool.ruff.lint.per-file-ignores]
"__init__.py" = ["F401"] # Ignore unused imports in init files -- we specify APIs this way
Copy link
Contributor

Choose a reason for hiding this comment

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

I am unsure if the generalization here is necessary, but I am ok with it.

Copy link
Contributor

@XzzX XzzX left a comment

Choose a reason for hiding this comment

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

We need to find a way how to deal with multiline strings also in black.

@XzzX XzzX merged commit c4fca19 into ruff Jan 2, 2025
18 of 19 checks passed
@XzzX XzzX deleted the not_as_ruff branch January 2, 2025 07:53
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