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

refactor: Change linting to Ruff #978

Merged
merged 11 commits into from
Nov 18, 2024
Merged

refactor: Change linting to Ruff #978

merged 11 commits into from
Nov 18, 2024

Conversation

hoxbro
Copy link
Member

@hoxbro hoxbro commented Nov 13, 2024

Aligning with our other repos.

Copy link

codecov bot commented Nov 13, 2024

Codecov Report

Attention: Patch coverage is 76.92308% with 3 lines in your changes missing coverage. Please review.

Project coverage is 87.36%. Comparing base (71e8e47) to head (5453327).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
param/parameterized.py 71.42% 2 Missing ⚠️
param/serializer.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #978      +/-   ##
==========================================
+ Coverage   87.26%   87.36%   +0.09%     
==========================================
  Files           9        9              
  Lines        4948     4939       -9     
==========================================
- Hits         4318     4315       -3     
+ Misses        630      624       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hoxbro hoxbro requested a review from maximlt November 13, 2024 11:12
param/parameterized.py Outdated Show resolved Hide resolved
tests/testdateparam.py Outdated Show resolved Hide resolved
@@ -26,7 +26,7 @@
# Allow this file to be used standalone if desired, albeit without JSON serialization
try:
from . import serializer
except ImportError:
except ModuleNotFoundError:
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'm not entirely sure about this one.

Copy link
Member Author

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

Suggested change
except ModuleNotFoundError:
except ImportError:

Copy link
Member Author

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...

❯ python -c "import param"  # with ModuleNotFoundError
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/shh/projects/holoviz/repos/param/param/__init__.py", line 4, in <module>
    from .depends import depends  # noqa: api import
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/shh/projects/holoviz/repos/param/param/depends.py", line 9, in <module>
    from .parameterized import (
  File "/home/shh/projects/holoviz/repos/param/param/parameterized.py", line 28, in <module>
    from . import serializer
ImportError: cannot import name 'serializer' from partially initialized module 'param' (most likely due to a circular import) (/home/shh/projects/holoviz/repos/param/param/__init__.py)

param on  refactor_ruff [$✘] via 🅒 holoviz
❯ python -c "import param"  # with ImportError
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/shh/projects/holoviz/repos/param/param/__init__.py", line 4, in <module>
    from .depends import depends  # noqa: api import
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/shh/projects/holoviz/repos/param/param/depends.py", line 9, in <module>
    from .parameterized import (
  File "/home/shh/projects/holoviz/repos/param/param/parameterized.py", line 1027, in <module>
    class Parameter(_ParameterBase):
  File "/home/shh/projects/holoviz/repos/param/param/parameterized.py", line 1170, in Parameter
    _serializers = {'json': serializer.JSONSerialization}
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'NoneType' object has no attribute 'JSONSerialization'

Copy link
Member Author

@hoxbro hoxbro Nov 14, 2024

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.

Copy link
Member

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

Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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!

Copy link
Member

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.

param/version.py Outdated Show resolved Hide resolved
@hoxbro hoxbro merged commit 6c28e65 into main Nov 18, 2024
16 checks passed
@hoxbro hoxbro deleted the refactor_ruff branch November 18, 2024 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.

4 participants