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

Use strict sorted and union for NoQA mapping insertion #7531

Merged
merged 1 commit into from
Sep 20, 2023
Merged

Conversation

dhruvmanila
Copy link
Member

@dhruvmanila dhruvmanila commented Sep 20, 2023

Summary

This PR fixes the way NoQA range is inserted to the NoqaMapping.

Previously, the way the mapping insertion logic worked was as follows:

  1. If the range which is to be inserted touched the previous range, meaning
    that the end of the previous range was the same as the start of the new
    range, then the new range was added in addition to the previous range.
  2. Else, if the new range intersected the previous range, then the previous
    range was replaced with the new intersection of the two ranges.

The problem with this logic is that it does not work for the following case:

assert foo, \
    """multi-line
    string"""

Now, the comments cannot be added to the same line which ends with a continuation
character. So, the NoQA directive has to be added to the next line. But, the
next line is also a triple-quoted string, so the NoQA directive for that line
needs to be added to the next line. This creates a union pattern instead of an
intersection pattern.

But, only union doesn't suffice because (1) means that for the edge case where
the range touch only at the end, the union won't take place.

Solution

  1. Replace '<=' with '<' to have a strict insertion case
  2. Use union instead of intersection

Test Plan

Add a new test case. Run the test suite to ensure that nothing is broken.

Integration

  1. Make a test.py file with the following contents:

    assert foo, \
        """multi-line
        string"""
  2. Run the following command:

    $ cargo run --bin ruff -- check --isolated --no-cache --select=F821 test.py
    /Users/dhruv/playground/ruff/fstring.py:1:8: F821 Undefined name `foo`
    Found 1 error.
  3. Use --add-noqa:

    $ cargo run --bin ruff -- check --isolated --no-cache --select=F821 --add-noqa test.py
    Added 1 noqa directive.
  4. Check that the NoQA directive was added in the correct position:

    assert foo, \
        """multi-line
        string"""  # noqa: F821
  5. Run the check command to ensure that the NoQA directive is respected:

    $ cargo run --bin ruff -- check --isolated --no-cache --select=F821 test.py

fixes: #7530

@dhruvmanila
Copy link
Member Author

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@dhruvmanila dhruvmanila added bug Something isn't working suppression Related to supression of violations e.g. noqa labels Sep 20, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Sep 20, 2023

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

@dhruvmanila dhruvmanila enabled auto-merge (squash) September 20, 2023 10:58
@dhruvmanila dhruvmanila merged commit 0a167dd into main Sep 20, 2023
15 checks passed
@dhruvmanila dhruvmanila deleted the dhruv/noqa branch September 20, 2023 11:07
renovate bot referenced this pull request in allenporter/flux-local Sep 24, 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://docs.astral.sh/ruff)
([source](https://github.com/astral-sh/ruff),
[changelog](https://github.com/astral-sh/ruff/releases)) | `==0.0.290`
-> `==0.0.291` |
[![age](https://developer.mend.io/api/mc/badges/age/pypi/ruff/0.0.291?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/pypi/ruff/0.0.291?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/pypi/ruff/0.0.290/0.0.291?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/pypi/ruff/0.0.290/0.0.291?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>astral-sh/ruff (ruff)</summary>

###
[`v0.0.291`](https://github.com/astral-sh/ruff/releases/tag/v0.0.291)

[Compare
Source](https://github.com/astral-sh/ruff/compare/v0.0.290...v0.0.291)

<!-- Release notes generated using configuration in .github/release.yml
at v0.0.291 -->

#### What's Changed

##### Deprecations

**The `format` command-line argument and configuration option has been
renamed to `output-format`.** While Ruff will continue to respect
`format` when passed as a command-line argument or configuration option,
this backwards-compatible support will be dropped in a future release.
See:
[https://github.com/astral-sh/ruff/pull/7514](https://github.com/astral-sh/ruff/pull/7514).

##### Rules

- \[`flake8-bandit`] Implement `S201`: `flask-debug-true` by
[@&#8203;mkniewallner](https://github.com/mkniewallner) in
[https://github.com/astral-sh/ruff/pull/7503](https://github.com/astral-sh/ruff/pull/7503)
- \[`flake8-bandit`] Implement `S507`: `ssh_no_host_key_verification` by
[@&#8203;mkniewallner](https://github.com/mkniewallner) in
[https://github.com/astral-sh/ruff/pull/7528](https://github.com/astral-sh/ruff/pull/7528)
- \[`flake8-logging`] Implement `LOG002`: `invalid-get-logger-argument`
by [@&#8203;dhruvmanila](https://github.com/dhruvmanila) in
[https://github.com/astral-sh/ruff/pull/7399](https://github.com/astral-sh/ruff/pull/7399)
- \[`flake8-logging`] Implement `LOG007`: `exception-without-exc-info`
by [@&#8203;qdegraaf](https://github.com/qdegraaf) in
[https://github.com/astral-sh/ruff/pull/7410](https://github.com/astral-sh/ruff/pull/7410)
- \[`refurb`] Implement `FURB140`: `reimplemented-starmap` by
[@&#8203;SavchenkoValeriy](https://github.com/SavchenkoValeriy) in
[https://github.com/astral-sh/ruff/pull/7253](https://github.com/astral-sh/ruff/pull/7253)
- \[`refurb`] Implement `FURB148`: `unnecessary-enumerate` by
[@&#8203;tjkuson](https://github.com/tjkuson) in
[https://github.com/astral-sh/ruff/pull/7454](https://github.com/astral-sh/ruff/pull/7454)
- \[`ruff`] Detect `asyncio.get_running_loop` calls in RUF006 by
[@&#8203;charliermarsh](https://github.com/charliermarsh) in
[https://github.com/astral-sh/ruff/pull/7562](https://github.com/astral-sh/ruff/pull/7562)

##### Settings

- Show `--no-X` variants in CLI help by
[@&#8203;charliermarsh](https://github.com/charliermarsh) in
[https://github.com/astral-sh/ruff/pull/7504](https://github.com/astral-sh/ruff/pull/7504)
- Rename `format` option to `output-format` by
[@&#8203;MichaReiser](https://github.com/MichaReiser) in
[https://github.com/astral-sh/ruff/pull/7514](https://github.com/astral-sh/ruff/pull/7514)
- Enable tab completion for `ruff rule` by
[@&#8203;charliermarsh](https://github.com/charliermarsh) in
[https://github.com/astral-sh/ruff/pull/7560](https://github.com/astral-sh/ruff/pull/7560)

##### Bug Fixes

- Add padding to prevent some autofix errors by
[@&#8203;charliermarsh](https://github.com/charliermarsh) in
[https://github.com/astral-sh/ruff/pull/7461](https://github.com/astral-sh/ruff/pull/7461)
- Remove parentheses when rewriting assert calls to statements by
[@&#8203;charliermarsh](https://github.com/charliermarsh) in
[https://github.com/astral-sh/ruff/pull/7464](https://github.com/astral-sh/ruff/pull/7464)
- Avoid flagging starred elements in C402 by
[@&#8203;charliermarsh](https://github.com/charliermarsh) in
[https://github.com/astral-sh/ruff/pull/7466](https://github.com/astral-sh/ruff/pull/7466)
- Extend `bad-dunder-method-name` to permit `attrs` dunders by
[@&#8203;tjkuson](https://github.com/tjkuson) in
[https://github.com/astral-sh/ruff/pull/7472](https://github.com/astral-sh/ruff/pull/7472)
- Avoid N802 violations for
[@&#8203;overload](https://github.com/overload) methods by
[@&#8203;JonathanPlasse](https://github.com/JonathanPlasse) in
[https://github.com/astral-sh/ruff/pull/7498](https://github.com/astral-sh/ruff/pull/7498)
- Avoid flagging starred expressions in UP007 by
[@&#8203;charliermarsh](https://github.com/charliermarsh) in
[https://github.com/astral-sh/ruff/pull/7505](https://github.com/astral-sh/ruff/pull/7505)
- Ensure that LOG007 only triggers on `.exception()` calls by
[@&#8203;charliermarsh](https://github.com/charliermarsh) in
[https://github.com/astral-sh/ruff/pull/7524](https://github.com/astral-sh/ruff/pull/7524)
- Use strict sorted and union for NoQA mapping insertion by
[@&#8203;dhruvmanila](https://github.com/dhruvmanila) in
[https://github.com/astral-sh/ruff/pull/7531](https://github.com/astral-sh/ruff/pull/7531)
- Avoid inserting imports directly after continuation by
[@&#8203;charliermarsh](https://github.com/charliermarsh) in
[https://github.com/astral-sh/ruff/pull/7553](https://github.com/astral-sh/ruff/pull/7553)
- Add padding in `PERF102` fixes by
[@&#8203;charliermarsh](https://github.com/charliermarsh) in
[https://github.com/astral-sh/ruff/pull/7554](https://github.com/astral-sh/ruff/pull/7554)
- Avoid invalid fix for parenthesized values in F601 by
[@&#8203;charliermarsh](https://github.com/charliermarsh) in
[https://github.com/astral-sh/ruff/pull/7559](https://github.com/astral-sh/ruff/pull/7559)
- Treat `os.error` as an `OSError` alias by
[@&#8203;charliermarsh](https://github.com/charliermarsh) in
[https://github.com/astral-sh/ruff/pull/7582](https://github.com/astral-sh/ruff/pull/7582)
- Extend `bad-dunder-method-name` to permit `__html__` by
[@&#8203;jaap3](https://github.com/jaap3) in
[https://github.com/astral-sh/ruff/pull/7492](https://github.com/astral-sh/ruff/pull/7492)
- Fix stylist indentation with a formfeed by
[@&#8203;konstin](https://github.com/konstin) in
[https://github.com/astral-sh/ruff/pull/7489](https://github.com/astral-sh/ruff/pull/7489)

#### New Contributors

- [@&#8203;MicaelJarniac](https://github.com/MicaelJarniac) made their
first contribution in
[https://github.com/astral-sh/ruff/pull/5498](https://github.com/astral-sh/ruff/pull/5498)
- [@&#8203;maheshsaripalli9](https://github.com/maheshsaripalli9) made
their first contribution in
[https://github.com/astral-sh/ruff/pull/7552](https://github.com/astral-sh/ruff/pull/7552)
- [@&#8203;T-256](https://github.com/T-256) made their first
contribution in
[https://github.com/astral-sh/ruff/pull/7585](https://github.com/astral-sh/ruff/pull/7585)

**Full Changelog**:
astral-sh/ruff@v0.0.290...v0.0.291

</details>

---

### Configuration

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

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, 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://developer.mend.io/github/allenporter/flux-local).

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

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working suppression Related to supression of violations e.g. noqa
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect NoQA placement for continuation with multi-line string
2 participants