Add new message called duplicate-value #5928

ksaketou · 2022-03-16T14:26:00Z

Type of Changes

	Type
✓	✨ New feature

Description

This PR creates a new error message called duplicate-value which identifies duplicate values inside sets.

Its functionality is similar to the duplicate-key message which finds duplicate values in dictionaries.

Closes #5880

This commit adds some new functionality which checks for duplicate values inside sets. For that reason, we create a new message called duplicate-value.

This commit adds new functional tests for the new message duplicate-value.

coveralls · 2022-03-16T14:40:49Z

Pull Request Test Coverage Report for Build 2079079540

11 of 11 (100.0%) changed or added relevant lines in 1 file are covered.
179 unchanged lines in 19 files lost coverage.
Overall coverage decreased (-0.008%) to 94.307%

Files with Coverage Reduction	New Missed Lines	%
pylint/checkers/design_analysis.py	1	98.9%
pylint/checkers/newstyle.py	1	96.15%
pylint/config/arguments_manager.py	1	97.62%
pylint/extensions/code_style.py	1	99.15%
pylint/extensions/comparetozero.py	1	97.06%
pylint/extensions/emptystring.py	1	96.77%
pylint/checkers/exceptions.py	3	97.7%
pylint/checkers/imports.py	4	94.39%
pylint/extensions/docparams.py	4	97.83%
pylint/extensions/typing.py	4	97.06%

Totals
Change from base Build 2078945924:	-0.008%
Covered Lines:	15438
Relevant Lines:	16370

💛 - Coveralls

nickdrozd · 2022-03-16T17:23:13Z

Tested locally; works! Thanks @ksaketou!

areveny

This looks pretty good. I think some additions would pick up many more common cases.

Also surfacing the failure on the Pylint presubmit: https://github.com/PyCQA/pylint/runs/5571708705?check_suite_focus=true

************* Module pylint.checkers.format
pylint/checkers/format.py:721:23: W0130: Duplicate value '\x0b' in set (duplicate-value)
pylint/checkers/format.py:721:23: W0130: Duplicate value '\x0c' in set (duplicate-value)

Which is this:

        unsplit_ends = {
            "\v",
            "\x0b",
            "\f",
            "\x0c",
            "\x1c",
            "\x1d",
            "\x1e",
            "\x85",
            "\u2028",
            "\u2029",
        }

https://stackoverflow.com/questions/26184100/how-does-v-differ-from-x0b-or-x0c

It seems \v == \x0b and \x0c == \f. I think we remove duplicates in our code. It can benefit clarity to explicitly write both synonym escape sequences, but that clarification is better in a comment.

pylint/checkers/base.py

ksaketou · 2022-03-20T19:00:14Z

It seems \v == \x0b and \x0c == \f. I think we remove duplicates in our code. It can benefit clarity to explicitly write both
synonym escape sequences, but that clarification is better in a comment.

So regarding that case should we modify unsplit_ends by keeping one of the two cases and referring to the other one in a comment? Which one should we keep in comments?

DanielNoord · 2022-03-21T08:09:54Z

It seems \v == \x0b and \x0c == \f. I think we remove duplicates in our code. It can benefit clarity to explicitly write both
synonym escape sequences, but that clarification is better in a comment.

So regarding that case should we modify unsplit_ends by keeping one of the two cases and referring to the other one in a comment? Which one should we keep in comments?

Let's keep the unicode one and add \v in the comments.

This commit changes the unsplit_ends set by keeping only the unicode sequences and noting their synonyms in comments in order to avoid duplicate-value error triggering.

DanielNoord · 2022-03-24T18:25:06Z

Hi @ksaketou,

We have been working on the documentation of the messages that pylint emits. As this PR adds a new message could I ask you to rebase on main and add two examples for bad and good code like explained in #5953? That way we immediately help users understand what this new message will do 😄

Pierre-Sassoulas · 2022-03-27T13:05:49Z

Hello @ksaketou, sorry the basic checker got split up on main in #5489 so the conflict resolution might be a little rough when rebasing on main. It's only code moving around so you can copy paste the code you added in the new files that were created.

ChangeLog

DanielNoord · 2022-03-29T09:10:45Z

doc/whatsnew/2.13.rst

@@ -66,6 +66,10 @@ New checkers

  Closes #5670

+* Added new message called ``duplicate-value`` which identifies duplicate values inside sets.


This should be moved to 2.14.

pylint/checkers/base.py

DanielNoord · 2022-03-29T09:12:15Z

pylint/checkers/base.py

+            else:
+                continue
+            if value in values:
+                self.add_message("duplicate-value", node=node, args=value)


Suggested change

self.add_message("duplicate-value", node=node, args=value)

self.add_message("duplicate-value", node=node, args=value, confidence=HIGH)

You'll probably need to import that.

ksaketou · 2022-03-29T14:38:17Z

Hello @DanielNoord @Pierre-Sassoulas , sorry for the late reply. Unfortunately, I don't currently have enough time to expand the checker's functionality (#5928 (comment)) . However, I can work on the documentation and the other minor suggested changes above during the following days if that's okay.

ksaketou · 2022-03-29T15:59:27Z

Quick question, should I add the documentation on a separate PR? @DanielNoord

DanielNoord · 2022-03-29T16:01:50Z

Quick question, should I add the documentation on a separate PR? @DanielNoord

No adding here is fine. It shouldn't increase the review burden significantly.

ksaketou · 2022-03-30T14:50:37Z

Did the refactoring affect the test execution at all? because I get errors regarding the setup.cfg.
I suppose this checker now goes into basic_checker.py?

When I add the timeout option into the markers config option the test passes, but I dont think I should modify this file.

Pierre-Sassoulas · 2022-03-30T17:46:51Z

Did the refactoring affect the test execution at all? because I get errors regarding the setup.cfg.

No it should be the same behavior, only less code per file.

I suppose this checker now goes into basic_checker.py?

Yes, I checked the diff it was in the BasicChecker class.

When I add the timeout option into the markers config option the test passes, but I dont think I should modify this file.

I had the same issue, I just fixed it in #6054, it should work if you rebase on latest main

DanielNoord

Almost there! Most of this looks good to go in my opinion!

ChangeLog

DanielNoord · 2022-04-01T07:17:02Z

doc/data/messages/d/duplicate-value/details.rst

@@ -0,0 +1,2 @@
+This error message is emitted when a set contains the same value two or more times.


Information like this should probably included in the message description in its declaration.

Only need to move this to pylint/checkers/base/basic_checker.py

doc/whatsnew/2.13.rst

Pierre-Sassoulas

LGTM, good job @ksaketou !

doc/data/messages/d/duplicate-value/good.py

ChangeLog

Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>

DanielNoord · 2022-04-01T18:12:00Z

doc/data/messages/d/duplicate-value/details.rst

@@ -0,0 +1,2 @@
+This error message is emitted when a set contains the same value two or more times.


Only need to move this to pylint/checkers/base/basic_checker.py

…into duplicate-value

ksaketou added 3 commits March 16, 2022 15:55

Add duplicate value check for sets

6e32e00

This commit adds some new functionality which checks for duplicate values inside sets. For that reason, we create a new message called duplicate-value.

Add new tests for duplicate-value message

656f7c1

This commit adds new functional tests for the new message duplicate-value.

Add duplicate-value message in Changelog and whats new

5a4b7e6

Pierre-Sassoulas added the Enhancement ✨ Improvement to a component label Mar 16, 2022

Pierre-Sassoulas added this to the 2.14.0 milestone Mar 16, 2022

Pierre-Sassoulas self-requested a review March 16, 2022 15:09

DanielNoord self-requested a review March 16, 2022 18:27

areveny suggested changes Mar 19, 2022

View reviewed changes

pylint/checkers/base.py Outdated Show resolved Hide resolved

pylint/checkers/base.py Outdated Show resolved Hide resolved

ksaketou added 2 commits March 21, 2022 22:41

Modify escape sequences clarification

1fbf4ba

This commit changes the unsplit_ends set by keeping only the unicode sequences and noting their synonyms in comments in order to avoid duplicate-value error triggering.

Resolve merge conflict

c55c08c

DanielNoord requested changes Mar 29, 2022

View reviewed changes

ksaketou added 2 commits March 29, 2022 18:12

Resolve merge conflict

6781ca0

Merge remote-tracking branch 'upstream/main' into duplicate-value

9f3e064

ksaketou added 4 commits March 31, 2022 17:53

Move doc to 2.14

a73f325

Merge remote-tracking branch 'upstream/main' into duplicate-value

21bea37

Move error to new file and change confidence to HIGH

ddd23e6

Add documentation for the new error

46a6085

DanielNoord requested changes Apr 1, 2022

View reviewed changes

Pierre-Sassoulas approved these changes Apr 1, 2022

View reviewed changes

doc/data/messages/d/duplicate-value/good.py Outdated Show resolved Hide resolved

ChangeLog Outdated Show resolved Hide resolved

Apply suggestions from code review

5ddddcb

Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>

Pierre-Sassoulas requested review from areveny and DanielNoord April 1, 2022 18:03

DanielNoord requested changes Apr 1, 2022

View reviewed changes

ksaketou and others added 4 commits April 1, 2022 21:13

Merge remote-tracking branch 'upstream/main' into duplicate-value

fc37a89

Modify message description

1798d36

Merge branch 'duplicate-value' of https://github.com/ksaketou/pylint …

3fb512b

…into duplicate-value

Remove file

e9103b1

DanielNoord approved these changes Apr 1, 2022

View reviewed changes

areveny approved these changes Apr 1, 2022

View reviewed changes

Pierre-Sassoulas merged commit 06bc0a6 into pylint-dev:main Apr 2, 2022

ksaketou deleted the duplicate-value branch April 2, 2022 09:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new message called duplicate-value #5928

Add new message called duplicate-value #5928

ksaketou commented Mar 16, 2022

coveralls commented Mar 16, 2022 •

edited

Loading

nickdrozd commented Mar 16, 2022

areveny left a comment

ksaketou commented Mar 20, 2022 •

edited

Loading

DanielNoord commented Mar 21, 2022

DanielNoord commented Mar 24, 2022

Pierre-Sassoulas commented Mar 27, 2022

DanielNoord Mar 29, 2022

DanielNoord Mar 29, 2022 •

edited by Pierre-Sassoulas

Loading

ksaketou commented Mar 29, 2022

ksaketou commented Mar 29, 2022

DanielNoord commented Mar 29, 2022

ksaketou commented Mar 30, 2022 •

edited

Loading

Pierre-Sassoulas commented Mar 30, 2022

DanielNoord left a comment

DanielNoord Apr 1, 2022

DanielNoord Apr 1, 2022

Pierre-Sassoulas left a comment

DanielNoord Apr 1, 2022

		@@ -66,6 +66,10 @@ New checkers

		Closes #5670

		* Added new message called ``duplicate-value`` which identifies duplicate values inside sets.

	self.add_message("duplicate-value", node=node, args=value)
	self.add_message("duplicate-value", node=node, args=value, confidence=HIGH)

		@@ -0,0 +1,2 @@
		This error message is emitted when a set contains the same value two or more times.

Add new message called duplicate-value #5928

Add new message called duplicate-value #5928

Conversation

ksaketou commented Mar 16, 2022

Type of Changes

Description

coveralls commented Mar 16, 2022 • edited Loading

Pull Request Test Coverage Report for Build 2079079540

💛 - Coveralls

nickdrozd commented Mar 16, 2022

areveny left a comment

Choose a reason for hiding this comment

ksaketou commented Mar 20, 2022 • edited Loading

DanielNoord commented Mar 21, 2022

DanielNoord commented Mar 24, 2022

Pierre-Sassoulas commented Mar 27, 2022

DanielNoord Mar 29, 2022

Choose a reason for hiding this comment

DanielNoord Mar 29, 2022 • edited by Pierre-Sassoulas Loading

Choose a reason for hiding this comment

ksaketou commented Mar 29, 2022

ksaketou commented Mar 29, 2022

DanielNoord commented Mar 29, 2022

ksaketou commented Mar 30, 2022 • edited Loading

Pierre-Sassoulas commented Mar 30, 2022

DanielNoord left a comment

Choose a reason for hiding this comment

DanielNoord Apr 1, 2022

Choose a reason for hiding this comment

DanielNoord Apr 1, 2022

Choose a reason for hiding this comment

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

DanielNoord Apr 1, 2022

Choose a reason for hiding this comment

coveralls commented Mar 16, 2022 •

edited

Loading

ksaketou commented Mar 20, 2022 •

edited

Loading

DanielNoord Mar 29, 2022 •

edited by Pierre-Sassoulas

Loading

ksaketou commented Mar 30, 2022 •

edited

Loading