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

Check for all set-length-modifying methods in modified-iterating-set (close #9334) #9333

Conversation

mdbernard
Copy link
Contributor

@mdbernard mdbernard commented Dec 29, 2023

  • Document your change, if it is a non-trivial one.
    • I would consider this change trivial, but this is my first PR to pylint, so please let me know if it would not be considered trivial by this repo's standards.
  • Relate your change to an issue in the tracker if such an issue exists
    • I did not find a related issue.
  • Write comprehensive commit messages and/or a good description of what the PR does.
  • Keep the change small, separate the consensual changes from the opinionated one.
    Don't hesitate to open multiple PRs if the change requires it. If your review is so
    big it requires to actually plan and allocate time to review, it's more likely
    that it's going to go stale.
  • If you used multiple emails or multiple names when contributing, add your mails
    and preferred name in script/.contributors_aliases.json

Type of Changes

Type
✨ New feature

Description

Currently, modified-iterating-set only checks for when the .add or .remove methods are called on the set being iterated over. However, .clear, .discard, and .pop also raise RuntimeError when used on a set while it is being iterated over, so there is value in linting them as well.

Closes #9334

@mdbernard mdbernard marked this pull request as ready for review December 29, 2023 06:34
@Pierre-Sassoulas Pierre-Sassoulas added the Enhancement ✨ Improvement to a component label Dec 29, 2023
@Pierre-Sassoulas Pierre-Sassoulas added this to the 3.1.0 milestone Dec 29, 2023
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Great contribution, thank you !

tests/functional/m/modified_iterating.py Outdated Show resolved Hide resolved
@Pierre-Sassoulas
Copy link
Member

Pierre-Sassoulas commented Dec 29, 2023

Here's an example of changelog : https://github.com/pylint-dev/pylint/blob/main/doc/whatsnew/fragments/8701.feature (you can refer to this merge request with Refs #9333 as there is no issue associated afaik)

Copy link

codecov bot commented Dec 29, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4329e05) 95.81% compared to head (6a983c4) 95.81%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #9333   +/-   ##
=======================================
  Coverage   95.81%   95.81%           
=======================================
  Files         173      173           
  Lines       18767    18767           
=======================================
  Hits        17981    17981           
  Misses        786      786           
Files Coverage Δ
pylint/checkers/modified_iterating_checker.py 97.72% <100.00%> (ø)

@mdbernard mdbernard changed the title Check for all set-length-modifying methods in modified-iterating-set Check for all set-length-modifying methods in modified-iterating-set (close #9334) Dec 29, 2023
@mdbernard mdbernard force-pushed the add-checks-to-modified-iterating-set branch from b907391 to 5860906 Compare December 29, 2023 17:22
@mdbernard mdbernard force-pushed the add-checks-to-modified-iterating-set branch from 6e8e76d to fecaf6b Compare December 29, 2023 17:28
@mdbernard
Copy link
Contributor Author

Here's an example of changelog : https://github.com/pylint-dev/pylint/blob/main/doc/whatsnew/fragments/8701.feature (you can refer to this merge request with Refs #9333 as there is no issue associated afaik)

(I made a new issue for this for ease of reference)

doc/whatsnew/fragments/9334.feature Outdated Show resolved Hide resolved
@Pierre-Sassoulas
Copy link
Member

Thank you for your work, very nice first contribution 👍 this is going to be released in 3.1.0, congratulations on becoming a pylint contributor ;)

@Pierre-Sassoulas Pierre-Sassoulas merged commit 4301129 into pylint-dev:main Dec 30, 2023
44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add checks for all set-length-modifying methods in modified-iterating-set
2 participants