Summary
--
This PR adds a new section to CONTRIBUTING.md describing the expected
contents of the PR summary and test plan, using the ecosystem report,
and communicating the status of a PR.
This seemed like a pretty good place to insert this in the document, at
the end of the advice on preparing actual code changes, but I'm
certainly open to other suggestions about both the content and
placement.
Test Plan
--
Future PRs :)
---------
Co-authored-by: Micha Reiser <micha@reiser.io>
Summary
--
During the release today, I noticed that the changelog is finally too
long to
render at all on GitHub. This PR follows the same splitting procedure as
in
uv (astral-sh/uv#11510, astral-sh/uv#12099): first splitting the file
into one
per minor version, and then reversing the contents of each file to start
with
the breaking release (`changelogs/0.11.x.md` starts with 0.11.0 instead
of
0.11.13 as in the old changelog).
For the second part, I used
[`reverse-changelog.py`](https://github.com/astral-sh/uv/blob/main/scripts/reverse-changelog.py)
from the uv repo, so hopefully everything is correct. I spot-checked
0.7.0 at least.
First take on a contributing guide for `ty`. Lots of it is copied from
the existing Ruff contribution guide.
I've put this in Ruff repo, since I think a contributing guide belongs
where the code is. I also updated the Ruff contributing guide to link to
the `ty` one.
Once this is merged, we can also add a link from the `CONTRIBUTING.md`
in ty repo (which focuses on making contributions to things that are
actually in the ty repo), to this guide.
I also updated the pull request template to mention that it might be a
ty PR, and mention the `[ty]` PR title prefix.
Feel free to update/modify/merge this PR before I'm awake tomorrow.
---------
Co-authored-by: Dhruv Manilawala <dhruvmanila@gmail.com>
Co-authored-by: David Peter <mail@david-peter.de>
## Summary
Noticed there was a wrong tip on the Contributing guide, `cargo
benchmark lexer` wouldn't run any benches.
Probably a missed update on #9535
It may make sense to remove the `cargo benchmark` command from the guide
altogether, but up to the mantainers.
## Summary
When following the step-by-step instructions to run the benchmarks in
`CONTRIBUTING.md`, I encountered two errors:
**Error 1:**
`bash: hyperfine: command not found`
**Solution**: I updated the instructions to include the step of
installing the benchmark tool.
**Error 2:**
```shell
$ ./target/release/ruff ./crates/ruff_linter/resources/test/cpython/
error: `ruff <path>` has been removed. Use `ruff check <path>` instead.
```
**Solution**: I added `check`.
## Test Plan
I tested it by running the benchmark-related commands in a new workspace
within GitHub Codespaces.
## Summary
Occasionally, we receive bug reports that imports in `src` directories
aren't correctly detected. The root of the problem is that we default to
`src = ["."]`, so users have to set `src = ["src"]` explicitly. This PR
extends the default to cover _both_ of them: `src = [".", "src"]`.
Closes https://github.com/astral-sh/ruff/issues/12454.
## Test Plan
I replicated the structure described in
https://github.com/astral-sh/ruff/issues/12453, and verified that the
imports were considered sorted, but that adding `src = ["."]` showed an
error.
## Summary
CONTRIBUTING.md says that `cargo dev print-ast` uses the old RuffPython
parser, even though, as far as I can tell, it uses the shiny new parser.
This PR fixes this.
## Test Plan
CI jobs should do the trick -- I didn't modify any code.
Reading through `CONTRIBUTING.md`, I happened to notice that it still
referred to the `ruff_linter` crate as the `ruff` crate. `ruff` is a
different crate, located in `crates/ruff`, which doesn't contain "the
vast majority of the code and all the lint rules."
## Summary
PEP 420 says [nested namespace
packages](https://peps.python.org/pep-0420/#nested-namespace-packages)
are allowed, i.e. marking a directory as a namespace package marks all
subdirectories in the subtree as namespace packages.
`is_package` is modified to use `Path::starts_with` and the order of
checks is reversed to do in-memory checks first before hitting the disk.
## Test Plan
Added unit tests. Previously all tests were run with `namespace_packages
== &[]`. Verified that one of the tests was failing before changing the
implementation.
## Future Improvements
The `is_package_with_cache` can probably be rewritten to avoid repeated
calls to `Path::starts_with`, by caching all directories up to the
`namespace_root`:
```ruff
let namespace_root = namespace_packages
.iter()
.filter(|namespace_package| path.starts_with(namespace_package))
.min();
```
Since i just spent quite some time wondering why my benchmarks were the
opposite of what they should be, a reminder to check your cpu governor.
Setting mine to perf mode was crucial.
## Summary
I noticed the example line in CONTRIBUTING.md:
```shell
cargo install nextest
```
Didn't appear to install the intended package cargo-nextest.
So I checked what it [should
be](https://nexte.st/book/installing-from-source.html) and replaced the
line:
```shell
cargo install cargo-nextest --locked
```
## Test Plan
Just checked the cargo install appeared to give sane looking results
---------
Co-authored-by: Charlie Marsh <charlie.r.marsh@gmail.com>
## Summary
We've had success with `nextest` in other projects, so lets migrate
Ruff.
The Linux tests look a little bit faster (from 2m32s down to 2m8s), the
Windows tests look a little bit slower but not dramatically so.
## Summary
This surprised me while working on adding a test. I thought about adding
an additional `note`, but how often is this incorrect? In general,
people reading the contributing guidelines probably want to enable this
flag and those who don't will know enough about the testing setup to
have their own commands/aliases.
## Test Plan
Ran CI on local fork and got an all green.
## Summary
Long ago, we had a single `ruff` crate. We started to break that up, and
at some point, we wanted to separate the CLI from the core library. So
we created `ruff_cli`, which created a `ruff` binary. Later, the `ruff`
crate was renamed to `ruff_linter` and further broken up into additional
crates.
(This is all from memory -- I didn't bother to look through the history
to ensure that this is 100% correct :))
Now that `ruff` no longer exists, this PR renames `ruff_cli` to `ruff`.
The primary benefit is that the binary target and the crate name are now
the same, which helps with downstream tooling like `cargo-dist`, and
also removes some complexity from the crate and `Cargo.toml` itself.
## Test Plan
- Ran `rm -rf target/release`.
- Ran `cargo build --release`.
- Verified that `./target/release/ruff` was created.
## Summary
I found that `cargo benchmark lexer` didn't work as expected:
```shell
❯ cargo benchmark lexer
Finished bench [optimized] target(s) in 0.08s
Running benches/formatter.rs (target/release/deps/formatter-4e1d9bf9d3ba529d)
Running benches/linter.rs (target/release/deps/linter-e449086ddfd8ad8c)
```
Turns out that `cargo bench -p ruff_benchmark` is now recommended over
`cargo benchmark`, so updating the docs to reflect that.
This should have been done before the actual release, so we add another
step to `CONTRIBUTING.md` to make sure it gets done in the future.
This doesn't fix https://github.com/astral-sh/ruff/issues/9234
completely, but it's a step in the right direction.
This sets `lto = "thin"` instead of using "fat" LTO, and sets
`codegen-units = 16`. These are the defaults for Cargo's `release`
profile, and I think it may give us faster iteration times, especially
when benchmarking. The point of this PR is to see what kind of impact
this has on benchmarks. It is expected that benchmarks may regress to
some extent.
I did some quick ad hoc experiments to quantify this change in compile
times. Namely, I ran:
cargo build --profile release -p ruff_cli
Then I ran
touch crates/ruff_python_formatter/src/expression/string/docstring.rs
(because that's where i've been working lately) and re-ran
cargo build --profile release -p ruff_cli
This last command is what I timed, since it reflects how much time one
has to wait between making a change and getting a compiled artifact.
Here are my results:
* With status quo `release` profile, build takes 77s
* with `release` but `lto = "thin"`, build takes 41s
* with `release`, but `lto = false`, build takes 19s
* with `release`, but `lto = false` **and** `codegen-units = 16`, build
takes 7s
* with `release`, but `lto = "thin"` **and** `codegen-units = 16`, build
takes 16s (i believe this is the default `release` configuration)
This PR represents the last option. It's not the fastest to compile, but
it's nearly a whole minute faster! The idea is that with `codegen-units
= 16`, we still make use of parallelism, but keep _some_ level of LTO on
to try and re-gain what we lose by increasing the number of codegen
units.
I'd rather not be the only one who can easily generate our changelog
entries so I invested some time to get Rooster a bit further along and
add instructions.
mahiro
Brent Westbrook
Amethyst Reese
Micha Reiser
Cornelius Roemer
renovate[bot]
Carl Meyer
Alex Waygood
Zanie Blue
Teodoro Freund
olp-cs
Mathieu Kniewallner
Charlie Marsh
Josh Cannon
Dhruv Manilawala
Evan Kohilas
Léopold Mebazaa
Carl Meyer
Alexey Preobrazhenskiy
Hoël Bagard
Jacob Coffee
konsti
Owen Lamont
Daniël van Noord
Mark Byrne
Jack McIvor
Andrew Gallant