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 with ruff #3482

Closed
wants to merge 4 commits into from

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

Copy link
Member

@agriyakhetarpal agriyakhetarpal left a comment

Choose a reason for hiding this comment

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

We should specifically use ruff.toml (docs) instead of pyproject.toml for now, since PyBaMM is not configured to support the pyproject.toml build format yet

@Rjchauhan18
Copy link
Contributor Author

We should specifically use ruff.toml (docs) instead of pyproject.toml for now, since PyBaMM is not configured to support the pyproject.toml build format yet

Can you please review it again?

ruff.toml Outdated
Comment on lines 1 to 2
[lint]
ignore = ["E402","E703"]
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
[lint]
ignore = ["E402","E703"]
[lint.per-file-ignores]
**.ipynb = ["E402", "E703"]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay it work if we use double quote and then **.ipynb inside it.

[lint.per-file-ignores]
"**.ipynb" = ["E402", "E703"]

hooks:
- id: ruff
types_or: [python, pyi, jupyter]
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.

Can you also move these configs to ruff.toml?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how can i do that ?

if using docs i add this to ruff.toml it not works

image

Copy link
Member

Choose a reason for hiding this comment

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

The documentation for ruff.toml (https://docs.astral.sh/ruff/configuration/#using-rufftoml) mentions "though in the ruff.toml and .ruff.toml versions, the [tool.ruff] header is omitted", which should help

Copy link
Contributor Author

Choose a reason for hiding this comment

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

test run smoothly but i don't have any effect on code even after i remove E703 from list of ignore.

it means after removing 10th line from pre-commit-config.yaml and improving ruff.toml is not working.

ruff.toml like this and with removed 10th line in pre-commit-config.yaml

image

Copy link
Member

Choose a reason for hiding this comment

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

@Rjchauhan18, it would be easier for us to help you if you push your changes (don't worry about the CI failing once you push it - we can help you fix the CI, but it becomes challenging to help you without looking at your entire changes).

Also, avoid adding screenshots of your logs; instead, copy-paste them into markdown (good practice).

@codecov
Copy link

codecov bot commented Oct 28, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (618b481) 99.58% compared to head (6cb6347) 99.58%.

❗ Current head 6cb6347 differs from pull request most recent head 5431dec. Consider uploading reports for the commit 5431dec to get more accurate results

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #3482   +/-   ##
========================================
  Coverage    99.58%   99.58%           
========================================
  Files          256      256           
  Lines        20048    20048           
========================================
  Hits         19965    19965           
  Misses          83       83           

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

@agriyakhetarpal
Copy link
Member

Note: this PR (like the earlier one as @Saransh-cpp had mentioned) is also from the develop branch of your fork, which is not recommended and would cause issues should you wish to contribute further with subsequent PRs

@Rjchauhan18
Copy link
Contributor Author

Note: this PR (like the earlier one as @Saransh-cpp had mentioned) is also from the develop branch of your fork, which is not recommended and would cause issues should you wish to contribute further with subsequent PRs

so do you want that i make PR from new branch? and close this one.

@agriyakhetarpal
Copy link
Member

Yes. You can cherry-pick the commits that are already here so that you don't lose any progress

@Saransh-cpp
Copy link
Member

No need to cherry-pick, you can simply checkout to a new branch, push the changes, and create a new PR -

# changes to a new branch
git checkout -b <my_new_branch_name>   # create and checkout to a new branch (the new branch will automatically get your commits from develop)
git push --set-upstream origin <my_new_branch_name>   # push your branch to your fork
create a pull request from github

# syncing your local develop branch
git remote add upstream https://github.com/pybamm-team/PyBaMM   # will add a new remote from which you'll be able to pull
git fetch --all   # fetches everything from origin (your fork) and upstream (original pybamm repo)
git checkout develop   # go back to develop
git reset --hard upstream/develop   # reset your local develop branch and make it the exact copy of pybamm's develop
git push -f   # force push to sync your fork's develop branch on GitHub with pybamm's develop branch

This might look a bit intimidating at start but you'll get used to the commands as you work more with git 🙂

@Rjchauhan18 Rjchauhan18 mentioned this pull request Oct 29, 2023
8 tasks
@agriyakhetarpal
Copy link
Member

Closing since #3483 has been opened

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.

3 participants