-
-
Notifications
You must be signed in to change notification settings - Fork 74
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
refactor: Change linting to Ruff #978
Merged
Merged
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
fd28589
refactor: Change lintint to Ruff
hoxbro 39bff9a
Fix bare except
hoxbro ef139d9
Fix E721
hoxbro 2e3998f
misc fixes
hoxbro 751641a
Update config files
hoxbro 38806e2
remove excepts
hoxbro ccaf158
ImportError -> ModuleNotFoundError
hoxbro 0f94534
Remove python 2 compatibility code
hoxbro 1284a34
Update param/parameterized.py
hoxbro 0d3f297
Update tests/testfiledeserialization.py
hoxbro 5453327
Remove try/except around serializer
hoxbro File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,21 +1,22 @@ | ||
default_stages: [pre-commit] | ||
repos: | ||
- repo: https://github.com/pre-commit/pre-commit-hooks | ||
rev: v4.3.0 | ||
- repo: https://github.com/pre-commit/pre-commit-hooks | ||
rev: v5.0.0 | ||
hooks: | ||
- id: check-builtin-literals | ||
- id: check-case-conflict | ||
- id: check-docstring-first | ||
- id: check-toml | ||
- id: detect-private-key | ||
- id: end-of-file-fixer | ||
- id: check-builtin-literals | ||
- id: check-case-conflict | ||
- id: check-docstring-first | ||
- id: check-toml | ||
- id: detect-private-key | ||
- id: end-of-file-fixer | ||
exclude: (\.min\.js$|\.svg$) | ||
- id: trailing-whitespace | ||
- repo: https://github.com/PyCQA/flake8 | ||
rev: 6.0.0 | ||
- id: trailing-whitespace | ||
- repo: https://github.com/astral-sh/ruff-pre-commit | ||
rev: v0.7.3 | ||
hooks: | ||
- id: flake8 | ||
- repo: https://github.com/hoxbro/clean_notebook | ||
rev: v0.1.10 | ||
hooks: | ||
- id: clean-notebook | ||
- id: ruff | ||
exclude: \.ipynb | ||
- repo: https://github.com/hoxbro/clean_notebook | ||
rev: v0.1.15 | ||
hooks: | ||
- id: clean-notebook |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely sure about this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested it. Needs to be an
ImportError
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though even with ImportError it still does not work...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove the try/except here? Nobody has complained about it, and that line was changed in June of 2021.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I've heard, and IIRC, Param was designed to be used super easily by simply copy-pasting one or two files in a project, which works as it has no external dependency. It's likely it made people's lives easier 10/15 years ago, these days we expect users to
pip/conda install
their dependencies and in the vast majority of cases it works fine.So I think that yes we should remove the
try/except
and generally lift the assumption that you can copy-paste some modules and it works, if it's baked in other places. I also wouldn't be surprised if one day we added some dependencies to Param (e.g.typing-extensions
if we finally do some typing work in Param).cc @philippjfr @jbednar @jlstevens
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maximlt is right in that param was originally designed to be easily vendored-in to any codebase. I personally have no strong feelings about this: while being easy to avoid an extra dependencies can be nice, it is also true that the majority of users will be using a package manager.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be sure, the question right now is not about adding an extra dependency but about removing the
try/except
around the serializer import, which has been broken since 2021.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, was just confirming what Maxime said and given this import has been broken so long, the issue is rather moot!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it is no longer important to be able to paste in the two main Param files directly into a project. Param was first written before conda and even before pip, and adding a dependency was much more of a big issue in those days. Happy to see this and any similar try/excepts removed, when they depend only on files in param itself.