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.

Sign up for GitHub

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

Jump to bottom

[pylint] Add PLE1141 DictIterMissingItems #9845

Merged
merged 8 commits into from
Feb 19, 2024
Merged

[pylint] Add PLE1141 DictIterMissingItems #9845

merged 8 commits into from
Feb 19, 2024

Conversation

DanielNoord
Copy link
Contributor

@DanielNoord DanielNoord commented Feb 5, 2024 •
edited by dhruvmanila
Loading

Summary

References #970.

Implements dict-iter-missing-items.

Took the tests from "upstream" here.

I wasn't able to implement code for one false positive, but it is pretty estoric: pylint-dev/pylint#3283. I would personally argue that adding this check as preview rule without supporting this specific use case is fine. I did add a "test" for it. This was implemented.

Test Plan

Followed the Contributing guide to create tests, hopefully I didn't miss any.
Also ran CI on my own fork and seemed to be all okay 😄

Edit: the ecosystem check seems a bit all over the place? 😅 All good.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 5, 2024 •
edited
Loading

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@dhruvmanila
Copy link
Member

Edit: the ecosystem check seems a bit all over the place? 😅

Could it be that the main branch on your fork is behind the main branch on this repository? IOW, can you try rebasing your branch with the latest changes on main?

@DanielNoord
Copy link
Contributor Author

@dhruvmanila A rebase did not seem to fix the issue. The edit of the comment is still the same. I don't see how this PR could have that ecosystem impact without any test related to these messages failing?

@dhruvmanila
Copy link
Member

@dhruvmanila A rebase did not seem to fix the issue. The edit of the comment is still the same. I don't see how this PR could have that ecosystem impact without any test related to these messages failing?

Yeah, don't worry about the ecosystem results.

@DanielNoord
Copy link
Contributor Author

Thanks for the feedback @dhruvmanila.

I force pushed as I made a clippy error in a previous commit. This should address your comments. Hopefully the ecosystem check can help out here as well :)

@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Feb 7, 2024 •
edited
Loading

CodSpeed Performance Report

Merging #9845 will not alter performance

Comparing DanielNoord:dict-iter-without-items (76d85ec) with main (b4f2882)

Summary

✅ 30 untouched benchmarks

dhruvmanila
dhruvmanila approved these changes Feb 7, 2024
View reviewed changes
Copy link
Member

@dhruvmanila dhruvmanila left a comment

Choose a reason for hiding this comment

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

Thanks!

@DanielNoord
Copy link
Contributor Author

Hi, @charliermarsh is there anything I can do to move this PR forward?

@dhruvmanila dhruvmanila added the rule Implementing or modifying a lint rule label Feb 19, 2024
@dhruvmanila
Copy link
Member

I think it's good to go. I'll merge it but feel free to review it and I can make any follow-up changes @charliermarsh

@dhruvmanila dhruvmanila changed the title Add PLE1141, DictIterMissingItems [pylint] Add PLE1141 DictIterMissingItems Feb 19, 2024
@dhruvmanila dhruvmanila merged commit 68b8abf into astral-sh:main Feb 19, 2024
17 checks passed
@charliermarsh
Copy link
Member

Thanks @dhruvmanila.

@BrewTestBot BrewTestBot mentioned this pull request Feb 29, 2024
Merged
nkxxll pushed a commit to nkxxll/ruff that referenced this pull request Mar 10, 2024
@DanielNoord @dhruvmanila
@charliermarsh @nkxxll
[pylint] Add PLE1141 DictIterMissingItems (astral-sh#9845)
da77e21 
## Summary

References astral-sh#970.

Implements
[`dict-iter-missing-items`](https://pylint.readthedocs.io/en/latest/user_guide/messages/error/dict-iter-missing-items.html).

Took the tests from "upstream"
[here](https://github.com/DanielNoord/pylint/blob/main/tests/functional/d/dict_iter_missing_items.py).

~I wasn't able to implement code for one false positive, but it is
pretty estoric: pylint-dev/pylint#3283. I
would personally argue that adding this check as preview rule without
supporting this specific use case is fine. I did add a "test" for it.~
This was implemented.

## Test Plan

Followed the Contributing guide to create tests, hopefully I didn't miss
any.
Also ran CI on my own fork and seemed to be all okay 😄 

~Edit: the ecosystem check seems a bit all over the place? 😅~ All good.

---------

Co-authored-by: Dhruv Manilawala <[email protected]>
Co-authored-by: Charlie Marsh <[email protected]>
@bswck bswck mentioned this pull request Mar 15, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhruvmanila dhruvmanila dhruvmanila approved these changes

@charliermarsh charliermarsh Awaiting requested review from charliermarsh

Assignees
No one assigned
Labels
rule Implementing or modifying a lint rule
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@DanielNoord @dhruvmanila @charliermarsh