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

chore: update pre-commit hooks #3290

Merged
merged 4 commits into from
Sep 11, 2023
Merged

Conversation

pre-commit-ci[bot]
Copy link
Contributor

@pre-commit-ci pre-commit-ci bot commented Aug 22, 2023

@codecov
Copy link

codecov bot commented Aug 22, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (c1c7c29) 99.70% compared to head (4da2b30) 99.70%.
Report is 9 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #3290   +/-   ##
========================================
  Coverage    99.70%   99.70%           
========================================
  Files          248      248           
  Lines        18894    18899    +5     
========================================
+ Hits         18839    18844    +5     
  Misses          55       55           
Files Changed Coverage Δ
pybamm/geometry/battery_geometry.py 100.00% <100.00%> (ø)
...m/models/full_battery_models/base_battery_model.py 99.54% <100.00%> (ø)
pybamm/models/submodels/base_submodel.py 100.00% <100.00%> (ø)
pybamm/parameters/base_parameters.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

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

Copy link
Member

@Saransh-cpp Saransh-cpp left a comment

Choose a reason for hiding this comment

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

Will check the failing CI

@Saransh-cpp Saransh-cpp self-requested a review August 23, 2023 23:08
@pre-commit-ci pre-commit-ci bot force-pushed the pre-commit-ci-update-config branch from 16cb21f to 3089f37 Compare August 29, 2023 02:51
@agriyakhetarpal
Copy link
Member

I just saw this one, I think we should ignore E721 here and keep using type() rather than using isinstance() to check types. I read up on this and this is because while isinstance() might be more Pythonic, it also checks across inheritance hierarchies (the type or subclasses of it) rather than just the type of the object at runtime. Though I guess you know more type theory than I do 😄

Nevertheless, this might be a bug we haven't caught because using isinstance() fails the unit tests too. I found this useful: https://stackoverflow.com/questions/21894575/isinstancefoo-bar-vs-typefoo-is-bar

updates:
- [github.com/astral-sh/ruff-pre-commit: v0.0.284 → v0.0.287](astral-sh/ruff-pre-commit@v0.0.284...v0.0.287)
- [github.com/adamchainz/blacken-docs: 1.15.0 → 1.16.0](adamchainz/blacken-docs@1.15.0...1.16.0)
@pre-commit-ci pre-commit-ci bot force-pushed the pre-commit-ci-update-config branch from beafc24 to d86e4a0 Compare September 5, 2023 03:37
@Saransh-cpp
Copy link
Member

I read up on this and this is because while isinstance() might be more Pythonic, it also checks across inheritance hierarchies

That is true, but the code is just checking if the variable is of type dict. The code breaking when an instance of any pybamm class is being type-checked would still make sense, but for Python classes, it should ideally not break.

Let's see if it breaks this time 🙁

@agriyakhetarpal
Copy link
Member

Oh I had tested it locally and it had failed, and it has now failed here as well. Do you know what a suitable fix would be?

@Saransh-cpp
Copy link
Member

Saransh-cpp commented Sep 11, 2023

The checks were failing as some PyBaMM classes are a subclass of dict. Suppressing the ruff errors should be okay in this case.

@Saransh-cpp Saransh-cpp merged commit 9612313 into develop Sep 11, 2023
30 of 32 checks passed
@Saransh-cpp Saransh-cpp deleted the pre-commit-ci-update-config branch September 11, 2023 19:26
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