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

Refactor unneeded-not checker #8789

Closed
orSolocate opened this issue Jun 21, 2023 · 9 comments · Fixed by #8846 or #9093
Closed

Refactor unneeded-not checker #8789

orSolocate opened this issue Jun 21, 2023 · 9 comments · Fixed by #8846 or #9093
Labels
Documentation 📗 Enhancement ✨ Improvement to a component Good first issue Friendly and approachable by new contributors Needs PR This issue is accepted, sufficiently specified and now needs an implementation
Milestone

Comments

@orSolocate
Copy link
Contributor

Current problem

A small suggestion to improve readability of this checker, since it covers not only the not keyword but also the != symbol.

Also in the description, it might not be perfectly clear what a unneeded negation is.

Desired solution

New name suggestion (or similar): redundant-negation

The description can be extended to this:

Used when a boolean expression contains an unneeded negation, e.g. when two negation operators cancel each other out.

Additional context

No response

@orSolocate orSolocate added the Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling label Jun 21, 2023
@Pierre-Sassoulas Pierre-Sassoulas added Enhancement ✨ Improvement to a component Documentation 📗 Good first issue Friendly and approachable by new contributors Needs PR This issue is accepted, sufficiently specified and now needs an implementation and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels Jun 21, 2023
@gracejiang16
Copy link
Contributor

Can I try this issue?

@DanielNoord
Copy link
Collaborator

Yes! Do you need any pointers to start working on it?

@orSolocate
Copy link
Contributor Author

@gracejiang16 yeah you can. should be super simple to change.

I am glad there is an agreement about the change itself

@gracejiang16
Copy link
Contributor

Yes! Do you need any pointers to start working on it?

Yes please thank you!

It seems like a simple search&replace, plus adding on to the description. Do I still need to write tests for this, or would I just make a pr?

@orSolocate
Copy link
Contributor Author

orSolocate commented Jun 28, 2023

  1. rename the checker name in its implementation file, under the pylint/checkers folder (should be able to find it using search).
  2. modify the description.
  3. find the test file, should be under tests/functional/u/unnecessary/unnecessary_not.py. modify its name, modify the test results, look at the guide (in pylint docs) of how to run functional tests with the option to autoupdate the text file. also verify the test covers enough cases.
  4. add more examples to bad/good.py codes located here:
    doc/data/messages/u/unneeded-not
  5. you can write a townscrier release note (see how to do it in the docs)

If you need more help we could try and help

@DanielNoord
Copy link
Collaborator

You will also need to use the "old name" system, which we use to deprecate old names and reuse new names without breaking people's setup. You can search for "missing-docstring" to see how this works.

@gracejiang16
Copy link
Contributor

You will also need to use the "old name" system, which we use to deprecate old names and reuse new names without breaking people's setup. You can search for "missing-docstring" to see how this works.

I've done everything up to the townscrier note. But I've tried reading through issue#1164, if that's what you mean, and I'm still having some trouble figuring out how to proceed. Do you have any tips or pointers? Thank you again.

@orSolocate
Copy link
Contributor Author

@gracejiang16 - I am not familiar with Daniel's note but I think that its best if you submit a PR connected to this issue, this way we could see what you did and could have a look.

@Pierre-Sassoulas
Copy link
Member

Do not hesitate to open a merge request even if something's is not working yet. Regarding old_names here's an example:

{"old_names": [("W1403", "implicit-str-concat-in-sequence")]},

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation 📗 Enhancement ✨ Improvement to a component Good first issue Friendly and approachable by new contributors Needs PR This issue is accepted, sufficiently specified and now needs an implementation
Projects
None yet
5 participants