Skip to content

Fix ruff #39

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

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Fix ruff #39

wants to merge 13 commits into from

Conversation

daklauss
Copy link
Member

@daklauss daklauss commented Mar 25, 2025

Pull request to fix the linter problem as described in #36 . Added the preview rules that may be unstable but now, ruff linter checks also for things like linespace etc. Also trying to update Ruff pipeline.

Closes #36

@daklauss daklauss force-pushed the fix-ruff branch 3 times, most recently from 47eb88e to c43ad89 Compare March 25, 2025 13:39
@daklauss daklauss requested a review from schmoelder March 25, 2025 13:40
@daklauss
Copy link
Member Author

The great formatting has been done. Added preview rules to the linter/formater. Ignored Rules that caused warnings. added pre-commit-config.yaml.

Also let ruff automaticaly by pre commit hook check and format linting stuff.

],
[
0, 1, 0, 1, 4, 5, 6, 7, 8, 9, 10,
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure how to adress this one ....... :/

'parameters_start': {
'inlet': {
'c': np.array([0., 1.]),
"parameters_start": {
Copy link
Member Author

Choose a reason for hiding this comment

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

Do you prefer single or double quotes?

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel that double quotes are more standard these days...

0, 1, 0, 1, 4, 5, 6, 7, 8, 9, 10,
11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21,
22, 23, 24, 25, 26, 22, 23
0,
Copy link
Member Author

Choose a reason for hiding this comment

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

Like one line for every number ? ... mkay

Copy link
Contributor

Choose a reason for hiding this comment

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

Add trailing comma to avoid line break. (See https://docs.astral.sh/ruff/settings/#format_skip-magic-trailing-comma)

Copy link
Member Author

Choose a reason for hiding this comment

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

This wont work for this case, because the entire line is to long. Trailing comma does not prevent line break if line is to long i guess....

state: str
) -> np.ndarray:
self, origin_list: list[(dict, float)], state: str
) -> np.ndarray:
Copy link
Contributor

Choose a reason for hiding this comment

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

Add trailing comma to avoid line break. (See https://docs.astral.sh/ruff/settings/#format_skip-magic-trailing-comma).

Copy link
Contributor

@schmoelder schmoelder left a comment

Choose a reason for hiding this comment

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

Good work!

Please add trailing commas to avoid line breaks in some function definitions.

def get_coupled_state(self,
origin_list: list[(dict, float)],
state: str
) -> np.ndarray:
Copy link
Contributor

Choose a reason for hiding this comment

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

Add trailing comma to avoid line break. (See https://docs.astral.sh/ruff/settings/#format_skip-magic-trailing-comma)

@daklauss
Copy link
Member Author

I removed the automated formatter in the pre commit file. If someone wants to use the autoformater he should handle it by himself with care. Otherwise i specified the directories to check (CadetPythonSim and tests). There a possibilities to ignore rules for specific files by setting:

[tool.ruff.per-file-ignores] 
"tests/**/*.py" = ["E128"]

This will be most likely necessary in Cadet Process.

@daklauss daklauss requested a review from schmoelder April 30, 2025 07:37
@daklauss daklauss force-pushed the fix-ruff branch 3 times, most recently from d836d57 to 37247c5 Compare April 30, 2025 08:22
@daklauss
Copy link
Member Author

daklauss commented May 5, 2025

Reworked the optional dependencies and using dependency groups for development related dependencies.

@@ -1,17 +1,10 @@
name: Ruff
on: [ push, pull_request ]
on: [ push ]
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remind me, was there a reason not to include this for PRs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not a strict reason. I guess i wanted to try to avoid duplicated execution of jobs. Otherwise it would execute this job twice if pushing on a pull_request.

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.

Improve formatting
2 participants