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

Add: ruff setup and instructions on autoformatting with vscode #34

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lwasser
Copy link
Member

@lwasser lwasser commented Jun 28, 2024

I think i need a bit of input on this pr - essentially it introduces pre-commit but also autosave (which has different formatters than ruff by itself.

I think we might want to setup a custom hatch environment to run tools (as an alternative to pre-commit). but i'm not 100% certain what is best here.

@@ -0,0 +1,98 @@
# Linting and Code Formatting
Copy link
Member Author

Choose a reason for hiding this comment

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

maybe add some discussion on hatch fmt (which only runs ruff). We want to run codespell and markdownlint and others potentially. What is the best way to do this?

https://hatch.pypa.io/latest/config/internal/static-analysis/

Copy link
Contributor

@blink1073 blink1073 Jun 28, 2024

Choose a reason for hiding this comment

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

I looked into using hatch fmt with Jupyter projects but decided against it, since without using https://hatch.pypa.io/latest/config/internal/static-analysis/#persistent-config the rules are opaque and not visible to editors, and I didn't want to commit such a large file to the repos.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok this makes sense. so we should for now, not use that.

how does jupyter handle linting or where do you all land?

want to set up pre-commit locally, or someone who wants to submit a pull request
(e.g., as a first contribution!) fully from the GitHub interface, you can enable
the bot to run pre-commit checks and fixes in the pull request yourself, as a
maintainer, before you merge the PR.
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing to bear in mind with this is that when the bot pushes a fix, contributors need to pull down that fix before making further changes, which can be confusing.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh right - good point! Would being a bit stronger about the language - only run pre-commit right before you merge the PR help clarify?

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant if the bot itself pushes a fix it can be confusing, which I don't think can be configured to run on demand.

Copy link
Member Author

Choose a reason for hiding this comment

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

AHHH ok. i do use the bot on demand - even in our repos here at pyos.

you can set the bot up to NOT autoformat and just check the repo. then when ready, use pre-commit autofix to call the bot.

my preferred workflow is

setup precommit.
Allow users to use it (or not)
i use it locally as a maintainer,
when a contributor submits a pr, i review and ignore linting until the end. when we are ready to merge, run precommit autofix on the pr. and the bot does it's thing.

the bot can't fix spelling errors.

what i'm trying to figure out here that i wonder if you / jupyter has solved is how should we suggest people run linters if pre-commit is confusing. (i understand it is confusing!)

Choose a reason for hiding this comment

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

Please remember also the security issues that @ofek mentioned on Slack. I think, that's important to note here.

issues in your files, it will not actually commit your files to history. This
can be confusing for even seasoned developers if they haven't used pre-commit
before.
* Some prefer to set up autoformatters that run every time you save a file in
Copy link
Contributor

Choose a reason for hiding this comment

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

We've mitigated this in some projects by relying on standard config as much as possible so that the scripts/IDEs can be run independently of pre-commit, and leaning on hatch environments for complex tasks.

Copy link
Member Author

Choose a reason for hiding this comment

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

so helpful - i think the same question as above - could you perhaps direct me to an example of how you run linters / formatter? i could see a hatch lint that we create or... ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Example in Jupyter Core:

  • ruff pre-commit hook and hatch run lint that runs those same hooks, to make sure we're calling the same version of ruff. We keep as much of the config as possible for the linters in pyproject.toml.
  • We have a similar setup there for typing


## Spyder

todo: add instructions here
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this is not yet implemented: spyder-ide/spyder#21357

Copy link
Member Author

Choose a reason for hiding this comment

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

thank you!!

},
},
// TODO: figure out how the formatter is selected for notebooks
"notebook.formatOnSave.enabled": true,
Copy link
Contributor

Choose a reason for hiding this comment

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

The Ruff VSCode Details page shows:

{
  "notebook.formatOnSave.enabled": true,
  "notebook.codeActionsOnSave": {
    "notebook.source.fixAll": "explicit",
    "notebook.source.organizeImports": "explicit"
  },
  "[python]": {
    "editor.defaultFormatter": "charliermarsh.ruff"
  }
}

Copy link
Member Author

Choose a reason for hiding this comment

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

thank you!!

Copy link

@Midnighter Midnighter left a comment

Choose a reason for hiding this comment

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

Overall, this is good advice that I wish I had when I started 😆. Made some comments, take them as you like.

autofix_prs: false
# Frequency of hook updates
autoupdate_schedule: weekly
autoupdate_schedule: quarterly

Choose a reason for hiding this comment

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

I'm in doubt about enabling auto update on a schedule. Wouldn't you want every contributor to run the same hook versions as in the main project branch? In my view, it's up to the maintainer to bump hook versions.

Choose a reason for hiding this comment

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

Do you still know someone who downloads PDFs of documentation these days? I'd be curious about usage numbers over all of readthedocs. I don't enable PDF on my projects any longer.

Comment on lines +36 to +40
* For beginner contributors, running pre-commit hooks can be confusing. You need
to understand that each time you commit, it will run the checks. If it finds
issues in your files, it will not actually commit your files to history. This
can be confusing for even seasoned developers if they haven't used pre-commit
before.

Choose a reason for hiding this comment

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

Suggested change
* For beginner contributors, running pre-commit hooks can be confusing. You need
to understand that each time you commit, it will run the checks. If it finds
issues in your files, it will not actually commit your files to history. This
can be confusing for even seasoned developers if they haven't used pre-commit
before.
* For beginner contributors, running pre-commit hooks can be confusing. You need
to understand that each time you commit, it will run the checks. If it finds
issues in your files, it will not actually commit your files to history. This
can be confusing for even seasoned developers if they haven't used pre-commit
before. If pre-commit hooks are not to your liking, please be aware that it is _your_
local development environment. You are not obliged to install them in the first place
or you can remove them with `pre-commit uninstall`. If you want to avoid the hooks
for a single commit, you can also skip them by running `git commit --no-verify`. Of course,
avoiding these hooks may mean that you later need to format your code by other means.

can be confusing for even seasoned developers if they haven't used pre-commit
before.
* Some prefer to set up autoformatters that run every time you save a file in
your preferred code editor (like VSCode). More on that below.

Choose a reason for hiding this comment

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

Suggested change
your preferred code editor (like VSCode). More on that below.
your preferred code editor (like VS Code). More on that below.

want to set up pre-commit locally, or someone who wants to submit a pull request
(e.g., as a first contribution!) fully from the GitHub interface, you can enable
the bot to run pre-commit checks and fixes in the pull request yourself, as a
maintainer, before you merge the PR.

Choose a reason for hiding this comment

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

Please remember also the security issues that @ofek mentioned on Slack. I think, that's important to note here.

Comment on lines +78 to +79
You can add this configuration to your workspace to ensure ruff is run on your
files every time you edit and save them.

Choose a reason for hiding this comment

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

Important to mention that there is also a UI for this. Open your settings, e.g., ctrl + , and search for ruff. You can also find it under Extensions. Choose to store settings either for your User or for the Workspace. Choose options like "Ruff > Lint: Run onSave" via the interface.

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