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

Split off ruff_cli crate from ruff library #1816

Merged
merged 1 commit into from
Jan 14, 2023

Conversation

not-my-profile
Copy link
Contributor

Please preserve the commits when merging.

@charliermarsh
Copy link
Member

Nice! This will be a big improvement.

Before I dig in, I just want to confirm: there are intended to be no behavior changes here, right? As in, Ruff itself should function identically? Purely an internal refactor?

@not-my-profile
Copy link
Contributor Author

Yes exactly.

ruff_cli/pyproject.toml Outdated Show resolved Hide resolved
ruff_cli/Cargo.toml Outdated Show resolved Hide resolved
@messense
Copy link
Contributor

I just checked locally, looks like maturin is having trouble with this kind of layout, mostly related to python-source option:

  • python/ruff/* isn't in the right place in sdist
  • python/ruff/* isn't packaged in wheel

I'll take a look tomorrow.

@charliermarsh
Copy link
Member

@not-my-profile - How big of a problem will it be for you if I make some changes to the pyproject.toml resolution code? (Specifically, to solve #1812.) I'm happy to wait until this is merged if it saves you a headache, though guessing that will be tomorrow or the next day and not today given the packaging needs.

@not-my-profile not-my-profile marked this pull request as draft January 12, 2023 17:21
@not-my-profile not-my-profile mentioned this pull request Jan 12, 2023
@not-my-profile
Copy link
Contributor Author

Thanks for asking :)

You could merge the first two commits from this PR first (I just opened #1822 for that). Then you can change the resolution code without causing headaches :) (The remaining two commits of this PR can be easily rebased.)

@charliermarsh
Copy link
Member

@not-my-profile - Hope you feel empowered to keep pushing on these sorts of refactors. They're making the project much stronger.

@charliermarsh
Copy link
Member

Will review (and hopefully merge) later today.

@charliermarsh
Copy link
Member

@not-my-profile - Can I squash the Clippy commit?

documentation = "https://github.com/charliermarsh/ruff"
homepage = "https://github.com/charliermarsh/ruff"
repository = "https://github.com/charliermarsh/ruff"
readme = "../README.md"
Copy link
Member

Choose a reason for hiding this comment

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

@messense - This was also necessary. As-is, ruff-0.0.220.dist-info/METADATA was missing the README contents.

@charliermarsh
Copy link
Member

charliermarsh commented Jan 14, 2023

Okay, the packages are looking more similar now:

Before:

❯ unzip -l /Users/crmarsh/workspace/ruff/target/wheels/ruff-0.0.220-py3-none-macosx_11_0_arm64.whl
Archive:  /Users/crmarsh/workspace/ruff/target/wheels/ruff-0.0.220-py3-none-macosx_11_0_arm64.whl
  Length      Date    Time    Name
---------  ---------- -----   ----
   120482  01-14-2023 01:34   ruff-0.0.220.dist-info/METADATA
      103  01-14-2023 01:34   ruff-0.0.220.dist-info/WHEEL
     1070  01-14-2023 01:34   ruff-0.0.220.dist-info/license_files/LICENSE
        0  01-14-2023 01:34   ruff/__init__.py
      193  01-14-2023 01:34   ruff/__main__.py
 40412178  01-14-2023 01:34   ruff-0.0.220.data/scripts/ruff
      540  01-14-2023 01:34   ruff-0.0.220.dist-info/RECORD

After:

❯ unzip -l /Users/crmarsh/workspace/ruff/target/wheels/ruff-0.0.220-py3-none-macosx_11_0_arm64.whl
Archive:  /Users/crmarsh/workspace/ruff/target/wheels/ruff-0.0.220-py3-none-macosx_11_0_arm64.whl
  Length      Date    Time    Name
---------  ---------- -----   ----
   120482  01-14-2023 01:37   ruff-0.0.220.dist-info/METADATA
      103  01-14-2023 01:37   ruff-0.0.220.dist-info/WHEEL
     1070  01-14-2023 01:37   ruff-0.0.220.dist-info/license_files/LICENSE
        0  01-14-2023 01:37   ruff/__init__.py
      193  01-14-2023 01:37   ruff/__main__.py
 38732738  01-14-2023 01:37   ruff-0.0.220.data/scripts/ruff
      540  01-14-2023 01:37   ruff-0.0.220.dist-info/RECORD

@charliermarsh
Copy link
Member

charliermarsh commented Jan 14, 2023

@not-my-profile - In fact, can we squash this to a single commit, with the message you want? I had to fix one bug (the missing README), then merged to resolve conflicts and verify that the contents on main were identical.

This lets you test the ruff linters or use the ruff library
without having to compile the ~100 additional dependencies
that are needed by the CLI.

Because we set the following in the [workspace] section of Cargo.toml:

   default-members = [".", "ruff_cli"]

`cargo run` still runs the CLI and `cargo test` still tests
the code in src/ as well as the code in the new ruff_cli crate.
(But you can now also run `cargo test -p ruff` to only test the linters.)
@not-my-profile
Copy link
Contributor Author

not-my-profile commented Jan 14, 2023

Since the first commit doesn't edit the files of the second commit and the clippy commit isn't that important, sure ... squashed into one :) Thanks for the fixes!

@charliermarsh charliermarsh merged commit 82aff5f into astral-sh:main Jan 14, 2023
@charliermarsh
Copy link
Member

Awesome - thank you both!

@not-my-profile
Copy link
Contributor Author

(I missed that we also have to update a command in the playground.yml workflow ... however wasm-pack build apparently doesn't support the new structure anyway ... I just opened #1860 to track that.)

renovate bot referenced this pull request in ixm-one/pytest-cmake-presets Jan 14, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [ruff](https://togithub.com/charliermarsh/ruff) | `^0.0.220` ->
`^0.0.221` |
[![age](https://badges.renovateapi.com/packages/pypi/ruff/0.0.221/age-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://badges.renovateapi.com/packages/pypi/ruff/0.0.221/adoption-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://badges.renovateapi.com/packages/pypi/ruff/0.0.221/compatibility-slim/0.0.220)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://badges.renovateapi.com/packages/pypi/ruff/0.0.221/confidence-slim/0.0.220)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>charliermarsh/ruff</summary>

###
[`v0.0.221`](https://togithub.com/charliermarsh/ruff/releases/tag/v0.0.221)

[Compare
Source](https://togithub.com/charliermarsh/ruff/compare/v0.0.220...v0.0.221)

#### What's Changed

- Document the way extend-ignore/select are applied by
[@&#8203;jankatins](https://togithub.com/jankatins) in
[https://github.com/charliermarsh/ruff/pull/1839](https://togithub.com/charliermarsh/ruff/pull/1839)
- Implement `PLR2004` (`MagicValueComparison`) by
[@&#8203;max0x53](https://togithub.com/max0x53) in
[https://github.com/charliermarsh/ruff/pull/1828](https://togithub.com/charliermarsh/ruff/pull/1828)
- Use absolute paths for --stdin-filename matching by
[@&#8203;charliermarsh](https://togithub.com/charliermarsh) in
[https://github.com/charliermarsh/ruff/pull/1843](https://togithub.com/charliermarsh/ruff/pull/1843)
- \[`flake8-bugbear`] Fix False Positives for `B024` & `B027` by
[@&#8203;saadmk11](https://togithub.com/saadmk11) in
[https://github.com/charliermarsh/ruff/pull/1851](https://togithub.com/charliermarsh/ruff/pull/1851)
- Clarify that some flake8-bugbear opinionated rules are already
implemented by [@&#8203;nsoranzo](https://togithub.com/nsoranzo) in
[https://github.com/charliermarsh/ruff/pull/1847](https://togithub.com/charliermarsh/ruff/pull/1847)
- \[`isort`] Add `classes` Config Option by
[@&#8203;saadmk11](https://togithub.com/saadmk11) in
[https://github.com/charliermarsh/ruff/pull/1849](https://togithub.com/charliermarsh/ruff/pull/1849)
- Implement `PLR0133` (`ComparisonOfConstants`) by
[@&#8203;max0x53](https://togithub.com/max0x53) in
[https://github.com/charliermarsh/ruff/pull/1841](https://togithub.com/charliermarsh/ruff/pull/1841)
- Remove non-magic trailing comma from tuple by
[@&#8203;charliermarsh](https://togithub.com/charliermarsh) in
[https://github.com/charliermarsh/ruff/pull/1854](https://togithub.com/charliermarsh/ruff/pull/1854)
- Improve spacing preservation for `C405` fixes by
[@&#8203;charliermarsh](https://togithub.com/charliermarsh) in
[https://github.com/charliermarsh/ruff/pull/1855](https://togithub.com/charliermarsh/ruff/pull/1855)
- Refactor import-tracking to leverage existing AST bindings by
[@&#8203;charliermarsh](https://togithub.com/charliermarsh) in
[https://github.com/charliermarsh/ruff/pull/1856](https://togithub.com/charliermarsh/ruff/pull/1856)
- Split off ruff_cli crate from ruff library by
[@&#8203;not-my-profile](https://togithub.com/not-my-profile) in
[https://github.com/charliermarsh/ruff/pull/1816](https://togithub.com/charliermarsh/ruff/pull/1816)
- Added ALE by [@&#8203;colin99d](https://togithub.com/colin99d) in
[https://github.com/charliermarsh/ruff/pull/1857](https://togithub.com/charliermarsh/ruff/pull/1857)
- Add workaround for wasm-pack bug to fix the playground CI by
[@&#8203;not-my-profile](https://togithub.com/not-my-profile) in
[https://github.com/charliermarsh/ruff/pull/1861](https://togithub.com/charliermarsh/ruff/pull/1861)
- Actually fix wasm-pack build command by
[@&#8203;not-my-profile](https://togithub.com/not-my-profile) in
[https://github.com/charliermarsh/ruff/pull/1862](https://togithub.com/charliermarsh/ruff/pull/1862)
- Avoid unnecessary allocations for module names by
[@&#8203;charliermarsh](https://togithub.com/charliermarsh) in
[https://github.com/charliermarsh/ruff/pull/1863](https://togithub.com/charliermarsh/ruff/pull/1863)

#### New Contributors

- [@&#8203;jankatins](https://togithub.com/jankatins) made their first
contribution in
[https://github.com/charliermarsh/ruff/pull/1839](https://togithub.com/charliermarsh/ruff/pull/1839)
- [@&#8203;max0x53](https://togithub.com/max0x53) made their first
contribution in
[https://github.com/charliermarsh/ruff/pull/1828](https://togithub.com/charliermarsh/ruff/pull/1828)
- [@&#8203;nsoranzo](https://togithub.com/nsoranzo) made their first
contribution in
[https://github.com/charliermarsh/ruff/pull/1847](https://togithub.com/charliermarsh/ruff/pull/1847)

**Full Changelog**:
astral-sh/ruff@v0.0.220...v0.0.221

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://app.renovatebot.com/dashboard#github/ixm-one/pytest-cmake-presets).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4xMDIuMCIsInVwZGF0ZWRJblZlciI6IjM0LjEwMi4wIn0=-->

Signed-off-by: Renovate Bot <bot@renovateapp.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
@charliermarsh
Copy link
Member

For reasons I don't fully understand, I think this change broke GitHub's dependency tracking:

Screen Shot 2023-01-14 at 3 00 48 PM

@charliermarsh
Copy link
Member

I'd really love to get this back. I don't understand why it broke as we still have the top-level setup.py.

@not-my-profile
Copy link
Contributor Author

Yeah I have also noticed that it's bugged ... when I refresh the page a couple of times it sometimes shows:

image

@charliermarsh
Copy link
Member

Oh, wait, I see that now too...

@not-my-profile
Copy link
Contributor Author

I don't think this PR changed any files that GitHub uses for dependency tracking ... maybe GitHub just cannot handle how many projects depend on ruff :P

@charliermarsh
Copy link
Member

charliermarsh commented Jan 15, 2023

Interestingly, I now see this, which toggles between the results. I wonder if there's any way to avoid the Rust version from showing up. Oh, maybe this is because we took the ruff crate?

Screen Shot 2023-01-15 at 2 32 14 AM

@charliermarsh
Copy link
Member

Ah, there's a setting for this! Ok, perfect.

Screen Shot 2023-01-15 at 2 37 01 AM

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