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

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 4 additions & 2 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,12 @@
# To run on all files: pre-commit run --all-files

ci:
# pyosMeta disables autofixing of PRs to simplify new contributor experience and cleaner git history
# We prefer to disable autofixing of PRs to simplify new contributor
# We use the autofix as an option before we merge to run linting if a conributor
# doesn't setup pre-commit (which can be confusing to new contributors)
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.


repos:
# Out of the box hooks for pre-commit https://github.com/pre-commit/pre-commit-hooks
Expand Down
4 changes: 0 additions & 4 deletions .readthedocs.yaml

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.

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 i'm responding to these comments in this closed PR (which i know is confusing!!).

This is a great question!!
My understanding is that PDFs are ideal for some people—especially those with less-than-good internet—because they can download once and read later offline. So that is why I generally include them. It is possible that for most people, this isn't an issue. But I've worked in areas where the internet is really challenging and students are on phones and tablets, and being able to download things is useful. we could chat more about this in a separate issue too!

Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,6 @@ build:
os: ubuntu-22.04
tools:
python: "3.12"
# You can also specify other tool versions:
# nodejs: "19"
# rust: "1.64"
# golang: "1.19"

# Build documentation in the "docs/" directory with Sphinx
sphinx:
Expand Down
98 changes: 98 additions & 0 deletions docs/documentation/code-format-lint.md
Original file line number Diff line number Diff line change
@@ -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?


In our [packaging guide](https://www.pyopensci.org/python-package-guide/package-structure-code/code-style-linting-format.html), we
discuss various linters and code formatters used in the scientific Python
ecosystem, including tools like:

* [black](https://github.com/psf/black)
* [flake8](https://github.com/pycqa/flake8)
* [isort](https://pycqa.github.io/isort/)

[Ruff](https://github.com/charliermarsh/ruff) is a tool that has been
quickly becoming more popular. It is written in the Rust programming language
and is fast. We like Ruff because it can perform both code formatting
(similar to Black) and linting (similar to Flake8). This makes your setup
simpler.

## Running Linters with pre-commit

There are a few ways to call the linters when you are working on your code.

One option is to use pre-commit hooks. [pre-commit](https://pre-commit.com/)
runs any defined linters, code and text formatters, spellcheckers, and other
tools on your code locally when you use `git commit` to make a change. For
example:

```bash
git commit -m "message here"
```

By configuring pre-commit hooks, you can automatically run tools like Ruff and
codespell on your code and documentation every time you commit. This ensures
consistency and catches errors early.

While pre-commit is a powerful tool to add to your workflow, many do not like it:

* 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.
Comment on lines +36 to +40

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed this locally!

* 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

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 for this @blink1073 !!

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed!


## Pre-commit.ci Bot

The [pre-commit CI bot](https://pre-commit.ci/) integrates with your GitHub
repository to run pre-commit checks in an online continuous integration pipeline.
The bot will run any pre-commit hooks that you have set up on new pull requests
submitted to your repository.

Pre-commit.ci can be a nice tool to use for pull requests if set up correctly.
Ideally, you can set pre-commit CI to run only when you call it in a pull
request. This way, if you have a new contributor (or a seasoned one) who doesn't
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.


## Setting Up Autosave

TODO: More here on setting this up in VSCode and other tools.

# Setting Up Autosave for Ruff in VSCode and Spyder

## VSCode

1. Make sure you have the the Python extension for vscode installed:

1. **Install Ruff:**
Ensure you have Ruff installed in your environment. You can install it using pip:

```bash
pip install ruff
```

1. Configure VSCode to run Ruff on save:

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

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.


```json
{
"[python]": {
"editor.formatOnSave": true,
"editor.defaultFormatter": "charliermarsh.ruff",
"editor.codeActionsOnSave": {
"source.fixAll": "always",
"source.organizeImports": "always",
},
},
// 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!!

}
```

## Spyder

Autoformat with ruff is not yet a Spyder feature. See [this issue](https://github.com/spyder-ide/spyder/issues/21357) for discussions.
File renamed without changes.