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

support more sophisticated fixme warnings #2874

Closed
ahvigil opened this issue Apr 18, 2019 · 15 comments
Closed

support more sophisticated fixme warnings #2874

ahvigil opened this issue Apr 18, 2019 · 15 comments
Labels
Enhancement ✨ Improvement to a component Good first issue Friendly and approachable by new contributors Hacktoberfest Help wanted 🙏 Outside help would be appreciated, good for new contributors Needs PR This issue is accepted, sufficiently specified and now needs an implementation
Milestone

Comments

@ahvigil
Copy link

ahvigil commented Apr 18, 2019

Is your feature request related to a problem? Please describe

pylint warns for configured fixme notes (defaulting to FIXME/XXX/TODO) but sometimes it would be desirable to be more flexible in how these are flagged. For example, # FIXME: this is broken by itself might not be ok but # FIXME: this is broken (ISSUE-1234) might be acceptable because it points to an externally tracked issue

Describe the solution you'd like

The notes config value should optionally allow a regex pattern to handle more complicated fixme conditions (eg FIXME*ISSUE-\d+).

Additional context

The checker for this rule already uses a regex pattern to flag configured notes so I think this should be easy to implement, but patterns are currently escaped before being inserted into the regex.

@PCManticore PCManticore added Enhancement ✨ Improvement to a component Good first issue Friendly and approachable by new contributors labels Apr 19, 2019
@PCManticore
Copy link
Contributor

Sounds good, thanks for reporting the issue.

@BennyTheSen
Copy link
Contributor

I will try to tackle this.
@PCManticore should this be done with an additional option like notes-rgx ?
I think the proposed solution to include regex in the notes option could cause confusion if someone is trying to detect comments like #./TODO without escaping regex specific tokens.

@PCManticore
Copy link
Contributor

@BennyTheSen Thanks for tackling this. Yes, a separate option would be the way to go. This maps nicely to the rest of our options that also accept a regex counterpart.

@Pierre-Sassoulas Pierre-Sassoulas added the Help wanted 🙏 Outside help would be appreciated, good for new contributors label Mar 2, 2021
@Pierre-Sassoulas Pierre-Sassoulas added the Needs PR This issue is accepted, sufficiently specified and now needs an implementation label Jul 4, 2022
@DanielNoord
Copy link
Collaborator

Fixed in 2.5 with #3394.

@DanielNoord DanielNoord closed this as not planned Won't fix, can't repro, duplicate, stale Jul 7, 2022
@DanielNoord DanielNoord added this to the 2.5 Release milestone Jul 7, 2022
@stdedos
Copy link
Contributor

stdedos commented Jul 7, 2022

I'd say "Closed as done"?

@stdedos
Copy link
Contributor

stdedos commented Jul 7, 2022

... Although do you mind explaining how does it work though? 😕

Assuming file:

"""test""

__version__ = "0.1"

# FixMe: test
# FixMe: ABC-1: test-abc
# FixMe: CBA-1: test-cba

and

notes-rgx=(FIXME|XXX|TODO)(?!.*ABC-\d+)

I expected:
image

but I got:

$  pylint src/__version__.py 
************* Module __version__
src/__version__.py:5:1: W0511: FixMe: test (fixme)
src/__version__.py:6:1: W0511: FixMe: ABC-1: test-abc (fixme)
src/__version__.py:7:1: W0511: FixMe: CBA-1: test-cba (fixme)

------------------------------------------------------------------
Your code has been rated at 0.00/10 (previous run: 0.00/10, +0.00)

"In this case" I'd like, apart from the existing functionality, a notes-rgx-ignore regex (i.e. drop any notes that also matches notes-rgx-ignore).

As you can appreciate, writing negative regex is not the funniest job on the planet - especially if the regex starts getting complicated.

Ofc, feel free to update documentation too ...

@DanielNoord
Copy link
Collaborator

Basically we warn whenever we find a pattern that matches the regex.
Schermafbeelding 2022-07-07 om 14 20 50

The above is an example of how I use this in one of my own projects. I want to allow TODO if there is an issue number associated with it, otherwise I want to flag.

I don't think we'll add an -ignore option. I know regex can be difficult, but I don't think there is anything that the current system does not allow you to do with a little regex crafting.

@stdedos
Copy link
Contributor

stdedos commented Jul 7, 2022

I don't think we'll add an -ignore option.

"Not that happy about that", but okay.

I still have a case that regex101 says should be okay, but pylint "refuses" to like it.
What gives? 😕

@DanielNoord
Copy link
Collaborator

That is because notes=FIXME as well.

[pylint]
notes-rgx=(FIXME|XXX|TODO)(?!.*ABC-\d+)
enable=fixme
notes=[]

This should work!

@stdedos
Copy link
Contributor

stdedos commented Jul 7, 2022

Also

[MISCELLANEOUS]

# List of note tags to take in consideration, separated by a comma.
notes="" # Remove the default entries

for the original config file

@DanielNoord
Copy link
Collaborator

Note that in 2.14 depending on the config file you're using you no longer needs those headers 😄
It might help you clean up your configuration file by removing useless headers!

@stdedos
Copy link
Contributor

stdedos commented Jul 7, 2022

pylint confuses me anyway so much, "it doesn't matter" 😓

I am using the .pylintrc file, "maybe sometime" I am planning to move it to pyproject.toml (if available)

@DanielNoord
Copy link
Collaborator

pylint confuses me anyway so much, "it doesn't matter" 😓

We're always happy to help with any issues 😄 The configurability of pylint has its downsides, making it a bit difficult to set up according to your wishes.

I am using the .pylintrc file, "maybe sometime" I am planning to move it to pyproject.toml (if available)

That is indeed available! You an even use the pylint-config command to make the file right now and take your current configuration with you!

@stdedos
Copy link
Contributor

stdedos commented Jul 7, 2022

Wuuuut? pylint has "auto-fix" things? 🤯 💥 (fix > migrate)

Thanks for letting me know 😄

@DanielNoord
Copy link
Collaborator

Wuuuut? pylint has "auto-fix" things? 🤯 💥 (fix > migrate)

Sorry to disappoint! I meant the configuration file. By using pylint-config you can generate a new configuration file based on your current configuration. That way you can easily upgrade from an pylintrc to an pyproject.toml file.

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 Hacktoberfest Help wanted 🙏 Outside help would be appreciated, good for new contributors Needs PR This issue is accepted, sufficiently specified and now needs an implementation
Projects
None yet
Development

No branches or pull requests

6 participants