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

Fix replace nbqa ruff to ruff #3483

Merged
merged 8 commits into from
Oct 31, 2023

Conversation

Rjchauhan18
Copy link
Contributor

Description

  • I have changed .pre-commit-config.yaml file and added ruff.toml for lint the jupyter notebook.

Fixes # (issue)

Type of change

  • New feature (non-breaking change which adds functionality)
  • Optimization (back-end change that speeds up the code)
  • Bug fix (non-breaking change which fixes an issue)

Key checklist:

  • No style issues: $ pre-commit run (or $ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)
  • All tests pass: $ python run-tests.py --all (or $ nox -s tests)
  • The documentation builds: $ python run-tests.py --doctest (or $ nox -s doctests)

You can run integration tests, unit tests, and doctests together at once, using $ python run-tests.py --quick (or $ nox -s quick).

Further checks:

  • Code is commented, particularly in hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

@Rjchauhan18
Copy link
Contributor Author

Hello @Saransh-cpp and @agriyakhetarpal
As you recommended i have created new branch and Created new PR for this issue #3476

@Rjchauhan18
Copy link
Contributor Author

Rjchauhan18 commented Oct 30, 2023

I think we needed to add this line types_or: [python, pyi, jupyter] in pre-commit-config.yaml as per...

NATIVE NOTEBOOK SUPPORT

Ruff 0.257 added experimental notebook support! You currently have to enable checking .ipynb files both in Ruff and pre-commit to use it.

This should be the 5431dec perfect changes and this line types_or: [python, pyi, jupyter] should be remain in pre-commit-config.yaml .

@@ -4,18 +4,11 @@ ci:

repos:
- repo: https://github.com/astral-sh/ruff-pre-commit
rev: "v0.1.1"
rev: "v0.1.3"
hooks:
- id: ruff
args: [--fix, --ignore=E741, --exclude=__init__.py]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
args: [--fix, --ignore=E741, --exclude=__init__.py]
args: [--fix, --show-fixes]

@@ -0,0 +1,4 @@
extend-include = ["*.ipynb"]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
extend-include = ["*.ipynb"]
extend-include = ["*.ipynb"]
extend-exclude = ["__init__.py"]
[lint]
ignore = [
"E741",
]

Copy link
Contributor Author

@Rjchauhan18 Rjchauhan18 Oct 31, 2023

Choose a reason for hiding this comment

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

if dont add types_or: [python, pyi, jupyter] this to pre-commit-config.yaml then it is not checking .ipynb

i mean if i remove "E703" from ruff.toml and then try to run the test then there is no changes in .ipynb it did not remove ;.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this should be expected, since we don't want to remove the semicolons

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.

Thanks, @Rjchauhan18! Looks great now!

@codecov
Copy link

codecov bot commented Oct 31, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e6eec08) 99.58% compared to head (f91fed9) 99.58%.
Report is 9 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #3483   +/-   ##
========================================
  Coverage    99.58%   99.58%           
========================================
  Files          256      256           
  Lines        20048    20048           
========================================
  Hits         19965    19965           
  Misses          83       83           
Files Coverage Δ
pybamm/solvers/jax_solver.py 90.69% <100.00%> (ø)

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

Woops, missed this

- id: nbqa-ruff
additional_dependencies: [ruff==0.0.284]
args: ["--fix","--ignore=E501,E402"]
args: [--fix, --show-fixes]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
args: [--fix, --show-fixes]
args: [--fix, --show-fixes]
types_or: [python, pyi, jupyter]

@Rjchauhan18
Copy link
Contributor Author

Hey @Saransh-cpp
I have participated in hactoberfest and hacksquad events could you please merge it as i have done all the neccessary changes.

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.

Thanks for working on this, @Rjchauhan18!

@Saransh-cpp Saransh-cpp linked an issue Oct 31, 2023 that may be closed by this pull request
@Saransh-cpp Saransh-cpp merged commit d37a8f2 into pybamm-team:develop Oct 31, 2023
32 of 33 checks passed
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.

Replace nbqa-ruff with ruff
3 participants