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.

Sign up for GitHub

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

Update pylint to version 3.0 #1310

Merged
merged 3 commits into from
Nov 2, 2023
Merged

Update pylint to version 3.0 #1310

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
22ba264
Update pylint to version 3.0
wshanks Nov 1, 2023
f339e9d
Merge branch 'main' into pylint
wshanks Nov 1, 2023
912e5cc
Remove parallelism from pylint runs in tox
wshanks Nov 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .pylintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,9 @@ disable=fixme, # disabled as TODOs would show up as warnings
no-else-return, # relax "elif" after a clause with a return
docstring-first-line-empty, # relax docstring style
import-outside-toplevel,
cyclic-import, # This checker raises on all module pairs that import each other,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we also turn off parallelism when running pylint to catch more errors potentially now that this check has been disabled?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure what else there is that can get caught with parallelism disabled. Our tests are slower so there is not much downside to removing parallelism for the CI runs (I see around 10 mins for test and under 3 for pylint with parallelism). Maybe it should be left on for a run someone would call locally, or at least easy to opt into. Or maybe people don't generally run it on all files locally any way (versus just on the file being edited). What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added a commit to remove the parallelism but make it possible to enable it optionally with tox by moving the posargs location. I don't think it's common to want extra arguments to black?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, that looks good! I haven't heard of people passing arguments to black. And I don't know specifically what removing parallelism might catch, but it seems fine since linting isn't that slow.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I actually ran into a reason to pass an argument to black. tox runs with --check without --diff. You might want to run with --diff to see what the changes would be. It's simple enough just to run -eblack --diff in that case though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

By the way, the time only changed from 2:46 to 2:54 for the two runs here with and without parallelism. There could be more difference locally on a system with more cores. The GitHub runners seem to have only two.

# even submodules that only import already loaded objects from a
# parent module, a common pattern in qiskit-experiments.
assigning-non-slot # https://github.com/Qiskit/qiskit-terra/pull/7347#issuecomment-985007311


Expand Down
4 changes: 2 additions & 2 deletions qiskit_experiments/framework/experiment_data.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ def _repr_svg_(self):
return None


_FigureT = Union[str, bytes, MatplotlibFigure, FigureData]
FigureT = Union[str, bytes, MatplotlibFigure, FigureData]


class ExperimentData:
Expand Down Expand Up @@ -1134,7 +1134,7 @@ def data(
@do_auto_save
def add_figures(
self,
figures: Union[_FigureT, List[_FigureT]],
figures: Union[FigureT, List[FigureT]],
figure_names: Optional[Union[str, List[str]]] = None,
overwrite: bool = False,
save_figure: Optional[bool] = None,
Expand Down
6 changes: 3 additions & 3 deletions requirements-dev.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
black~=22.0
stestr
pylint~=2.16.2
astroid~=2.14.2 # Must be kept aligned to what pylint wants
pylint~=3.0.2
astroid~=3.0.1 # Must be kept aligned to what pylint wants
jinja2==3.0.3
sphinx>=6.2.1,<=7
jupyter-sphinx>=0.4.0
Expand All @@ -20,4 +20,4 @@ coverage>=5.5
ipykernel<=6.21.3
jupyter-client<=8.0.3
ipython<8.13.0 ; python_version<"3.9" # for python 3.8 compatibility
sphinx-remove-toctrees
sphinx-remove-toctrees