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 sets for duplicate entries #5880

Closed
nickdrozd opened this issue Mar 8, 2022 · 19 comments · Fixed by #5928
Closed

Check sets for duplicate entries #5880

nickdrozd opened this issue Mar 8, 2022 · 19 comments · Fixed by #5928
Assignees
Labels
Enhancement ✨ Improvement to a component Good first issue Friendly and approachable by new contributors Help wanted 🙏 Outside help would be appreciated, good for new contributors
Milestone

Comments

@nickdrozd
Copy link
Collaborator

Current problem

Pylint check dictionaries for duplicates, but not sets.

DICTIONARY = {
    1: 2,
    2: 3,
    1: 2,  # duplicate flagged
}

SET = {
    1,
    2,
    1,  # duplicate not flagged
}

Desired solution

Flag duplicate set entries just as with dictionaries.

Additional context

No response

@nickdrozd nickdrozd added the Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling label Mar 8, 2022
@Pierre-Sassoulas Pierre-Sassoulas added Enhancement ✨ Improvement to a component Help wanted 🙏 Outside help would be appreciated, good for new contributors Good first issue Friendly and approachable by new contributors and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels Mar 8, 2022
@ksaketou
Copy link
Contributor

Hello, I'd like to work on that. If I understood correctly, the message we are talking about here is duplicate-key?

@DanielNoord
Copy link
Collaborator

Yes! We might also need to update the description of the message to not only mention dictionaries any more 😉

@ksaketou
Copy link
Contributor

Okayy, so we don't have to create a new message for the sets?

@DanielNoord
Copy link
Collaborator

DanielNoord commented Mar 11, 2022

Hm, now that I think about it, perhaps we should create duplicate-elements? The original pep refers to the entries in a set as values and elements. Using keys might be a bit confusing? What do others think?

@Pierre-Sassoulas
Copy link
Member

I think creating a new message duplicate-values make sense.

@nickdrozd
Copy link
Collaborator Author

The PEP says:

Programmers are often told that they can implement sets as dictionaries with “don’t care” values. Items can be added to these “sets” by assigning the “don’t care” value to them; membership can be tested using dict.has_key...

From this perspective, set entries are more like "keys". But then the rest of the PEP refers to them as "values". And the docstring for set mentions "elements", as is usually done in math.

So in the interest of covering every perspective, I suggest we use duplicate-kvalemuys.

@ksaketou
Copy link
Contributor

Hello, I am trying to run the tests but I get a ModuleNotFoundError at the primer tests for this line:
https://github.com/PyCQA/pylint/blob/8e9a44727545b09ccc0161d72b8b66bc92d95d88/pylint/testutils/primer.py#L5
Any idea why this is happening?

@Pierre-Sassoulas
Copy link
Member

Hello @ksaketou, you need to install pylint locally with pip3 install -e .[testutil] we made this package an extra in #5486 in order to not add a runtime dependency for standard users.

@ksaketou
Copy link
Contributor

Hello again, I'm getting an unexpected syntax-error at the symlink_module1 functional test, but I don't know how my changes could be related to this specific error message. I believe this is the line with the problem:
https://github.com/PyCQA/pylint/blob/975550582494f9e1c0eb43336943c716120bc4e6/tests/functional/s/symlink/_binding/symlink_module.py#L1

@DanielNoord
Copy link
Collaborator

How did you clone the repository? That functional test is dependent on symlinks so I think it is important that the repository is cloned via git instead of installed via pip or downloaded directly from Github.

@ksaketou
Copy link
Contributor

I cloned it with git clone. The path of this line is correct though, it's not that I miss any files.

@DanielNoord
Copy link
Collaborator

Did you get a output log for the syntax error_

@ksaketou
Copy link
Contributor

That's the output I get.
image

Pylance also points the line as an error with the message Expected expression.
image

@DanielNoord
Copy link
Collaborator

DanielNoord commented Mar 15, 2022

Hm, you might need to reclone your repository. That file should not show ../symlink_module/symlink_module.py but rather the content of ../symlink_module/symlink_module.py , which is:

"""Example taken from issue #1470"""


def func():
    """Both module should be parsed without problem"""
    return 1

I believe git should automatically copy any symlinks, but apparently it failed to do so for you locally. (CI passes on Windows so it should work I guess?...)

The files should show up like this in the VSC File Explorer, to indicate that they are symlinked.

Schermafbeelding 2022-03-15 om 14 23 23

Edit: Some googling showed that this might be relevant:
https://gh.neting.ccmunity/t/git-bash-symbolic-links-on-windows/522/4
https://stackoverflow.com/questions/5917249/git-symlinks-in-windows
https://github-wiki-see.page/m/git-for-windows/git/wiki/Symbolic-Links
https://www.quora.com/How-does-Git-handle-symbolic-links-on-Windows

@ksaketou
Copy link
Contributor

I have two files. The _binding/symlink_module.py contains the path I showed you above and the symlink_module/symlink_module.py contains this:

"""Example taken from issue #1470"""


def func():
    """Both module should be parsed without problem"""
    return 1

And these are the files at my Explorer:
image
Thank you for the links, I will try to fix this :)

@ksaketou
Copy link
Contributor

Keeping the symlink as a string like below seems to work for my case so far. However, I don't know if there is any problem with changing this file.

image

@DanielNoord
Copy link
Collaborator

Yeah, if the file gets changed then the test won't work anymore. Perhaps you can revert the change and see if you can commit without any changes to the file?

Were you unable to allow git to create symlinks?

@ksaketou
Copy link
Contributor

When I run the symlink_module1 test with that change it passes though.

I changed the core.symlinks of the .gitconfig file to True but that didn't seem to work. If I don't find anything else I will try to commit without that change as you said.

@DanielNoord
Copy link
Collaborator

I'll need to see the actual commit to see if nothing actually changes. This is difficult to interpret without seeing the actual changes git picks up on. Luckily we have the CI which should also pick up on this 😄

I'd say go ahead with working on this, we should be able to resolve any issues with this particular test if they arise!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component Good first issue Friendly and approachable by new contributors Help wanted 🙏 Outside help would be appreciated, good for new contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants