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

Show why snake_case errors is emitted on single word variables (might be too small) #2018

Closed
rmullin7286 opened this issue Apr 15, 2018 · 39 comments · Fixed by #8813 or #9093
Closed
Assignees
Labels
Bug 🪲 Hacktoberfest High priority Issue with more than 10 reactions Needs PR This issue is accepted, sufficiently specified and now needs an implementation
Milestone

Comments

@rmullin7286
Copy link

Steps to reproduce

no = 'No'

Current behavior

I'm getting an error saying that 'no' doesn't conform the camel_case style, even though it's only one word.

Expected behavior

There should be no error.

pylint --version output

pylint 1.8.4,
astroid 1.6.3
Python 3.6.3 (v3.6.3:2c5fed8, Oct 3 2017, 18:11:49) [MSC v.1900 64 bit (AMD64)]

@PCManticore
Copy link
Contributor

Hi, thanks for creating this issue! Can you also post your pylintrc file? You could also run pylint with --include-naming-hint=y, which will offer more indication on why it considers it a bad name. I think it might be because the name it's too short, by default we expect words of length 3+.

@PCManticore PCManticore changed the title getting snake_case errors on single word variables Show why snake_case errors is emitted on single word variables (might be too small) Apr 17, 2018
@eight04
Copy link

eight04 commented May 10, 2018

Is there a way to configure the minimum length of the variable name?

@PCManticore
Copy link
Contributor

@eight04 Not quite, the only thing you could do is to change the variable-rgx to conform to the format and length you require. It might be a good idea to just separate the length of a variable/name in pylint from the format of a variable/name.

@gitaarik
Copy link

gitaarik commented Dec 11, 2018

I tried setting variable-rgx, but the invalid-name snake_case error doesn't go away. The variable-rgx setting does work for variables that really don't conform to the regex, but it doesn't take away the first warning, although the docs say that it should override the variable-naming-style setting. When I try to unset it with: variable-naming-style= then both errors go away. All by all, it doesn't make any difference for me.

pylint 2.1.1
astroid 2.0.4
Python 3.6.7 (default, Oct 22 2018, 11:32:17) 
[GCC 8.2.0]

EDIT:

Oh I found out, I got this error for a function argument, and that's a different option:

argument-rgx=^[a-z][a-z0-9]*((_[a-z0-9]+)*)?$

This works for me :)!

@filips123
Copy link

Why is variable length important at all?
Variable can have good name even if it is short. Example for this can be no and some physical variables and constants.
What else can be done excluding changing variable-rgx? One name that conform this style is var_no, but is even worse then just no.

@pedro-w
Copy link

pedro-w commented Jan 3, 2019

I agree with @filips123 - I've been using Python to do physics calculations and I use vars like m, x and y to match the equations. It looks strange and wordy to have to rename them mass, x_coordinate etc.
One option is to use the good-names= config item but it's a bit tedious to maintain in practice.

@PCManticore
Copy link
Contributor

@pedro-w and @filips123 If variable-rgx or good-names is not good for you, you can also disable the check locally in places where you get this error. I don't think x m and similar variable names are readable to other people other than the one that wrote the code, so be mindful of this aspect as well. As it's mentioned in the documentation, pylint is not a one size fits it all, if something does not work for you, feel free to disable / adapt it to your needs.

@filips123
Copy link

Is it possible to specify variable-rgx or good-names in file (with inline comment) without pylintrc file?

@PCManticore
Copy link
Contributor

No, that's not possible.

@filips123
Copy link

Why? Can you add support for this? It would be useful if only one file contains that variables.

@PCManticore
Copy link
Contributor

Because it requires changing how we do configuration, it's a high effort project that is not worth it for almost all the cases.

@matkoniecz
Copy link

matkoniecz commented Jan 28, 2019

To give more specific solution for people who will run into this bug:

pylint t.py --variable-rgx=^[a-z][a-z0-9]*((_[a-z0-9]+)*)?$ --argument-rgx=^[a-z][a-z0-9]*((_[a-z0-9]+)*)?$

will stop complaining about perfectly good names. BTW, it would be nice to have solution similar to Rubocop config files - where it is easy to exclude complaints about names that make perfect sense (and some like x and y are predefined)

See matkoniecz/matkoniecz-ruby-style@e2df9de (how people may configure allowed names) and rubocop/rubocop#6137 "Allow db to allowed names of Naming/UncommunicativeMethodParamName cop in default config"

@PCManticore
Copy link
Contributor

@matkoniecz That's most likely an use case for good-names

@blaiseli
Copy link

blaiseli commented Oct 4, 2019

Reading the above discussion, I learned about the good-names option, and I'm grateful for it, but I'm not sure I understand whether something is already under way to change the way pylint reports about short variable names or not, and if yes, what will be the new behaviour.

I'm using a freshly installed pylint (using pip under python3.7):

$ pylint --version
pylint 2.4.2
astroid 2.3.1
Python 3.7.2 (default, Mar  6 2019, 18:38:11) 
[GCC 4.9.0]

With this version, variable name fh gets caught as Variable name "fh" doesn't conform to snake_case naming style. f_h does not (which I indeed consider snake_case, so everything fine here), but fhh too does not trigger a message. This means that (fortunately) the use of underscore is not mandatory. So the problem with fh is not really that it is not snake_case, but that it is too short.

I may be remembering wrong, but it seems to me that some time ago, pylint would not report about snake_case naming style in the case of short variable names, but either more generally, about bad naming style, or more precisely, about the shortness of the name. I would argue the latter would be desirable as new behaviour. But maybe it is complicated to obtain if the names are checked against just one "one-size-fits-all" regexp.

@demongolem
Copy link

demongolem commented Nov 14, 2019

I just ask a clarification. I thought it was standard name to use f for file objects and hence I am getting these warnings in a few different places. Would there be a more acceptable name for a file object, or do I somehow have to deal with these myself?

@chrissshe
Copy link

@demongolem Reading above, I think you just need to somehow have to deal with these myself, which is really straightforward.

Add f to good-names See reference at http://pylint.pycqa.org/en/latest/technical_reference/features.html?highlight=good-names#basic-checker-options

@chrissshe
Copy link

I got this warning even for stuff like for i in range(100) . Does pylint advise against using i?

@PCManticore
Copy link
Contributor

@chrissshe Not by default, i is a permitted name among other, but you might have overwritten the default good-names list of permitted names if you are still getting this error for i.

@chrissshe
Copy link

@PCManticore That's probably the case. I edited good-names to allow df (dataframe)

@prjemian
Copy link

Because it requires changing how we do configuration, it's a high effort project that is not worth it for almost all the cases.

I understand that it may be a high effort project, thus remain patient for it to stay on the long-term road map for implementation. The remark about not worth it for almost all the cases is opinion and is at odds with a substantial community.

In physics, common coding practice will often adopt names used in textbooks or published literature as being best choice. These names are recognized internationally, even though they rely on domain-specific jargon, it is exactly the entirety of that community they reach.

As we recommend value-added steps, such as linting, to coding practice for our non-programmer peers, we must advise them which of these findings to either heed or ignore. For sure, none of these pylint novices will apply local configurations or regexp rules. As it stands now, a pylint report is rife with such false positive findings. We insist to our novices that these reports are only advisory.

A command-line option would be of great benefit to a significant community.

@PCManticore
Copy link
Contributor

@prjemian Thanks for the comment, but I'm not sure why --good-names is not enough for you at this point? If f, df or whatever name you want to use still emits this error, simply passing --good-names=f,df to pylint should be enough to remove the error, without using a local configuration, nor regexp rule.

Also I'm not sure you're quoting me on the right context. The quoted answer was related to #2018 (comment), which suggests to add inline comments for good-names and similar options, which as far as I can tell, will not happen anytime soon, hence the too much effort comment of a change that is not worth doing.

I'm not sure what your comment is suggesting though. Is it the inline comment support? Is it something else? Right now this issue is about showing why a short variable might not match the naming style, namely because it is too short, not because it does not match the chosen naming style.

@prjemian
Copy link

Because it requires changing how we do configuration, it's a high effort project that is not worth it for almost all the cases.

@PCManticore : This was your quote, right?

I missed that it referred to markup in the python file that identified potential good names.


Thanks to your reply, investigated further the --good-names option and agree that it will work. Requires at least one pass to identify the flagged names, then exclude them, such as:

(py37) jemian@wow ~/.../projects/xpcs-nexus-8idi $ pylint load_xpcs_result.py
************* Module load_xpcs_result
load_xpcs_result.py:19:42: C0103: Variable name "f" doesn't conform to snake_case naming style (invalid-name)
load_xpcs_result.py:20:8: C0103: Variable name "Iq" doesn't conform to snake_case naming style (invalid-name)
load_xpcs_result.py:23:8: C0103: Variable name "t0" doesn't conform to snake_case naming style (invalid-name)
load_xpcs_result.py:25:8: C0103: Variable name "g2" doesn't conform to snake_case naming style (invalid-name)
load_xpcs_result.py:27:8: C0103: Variable name "Int_2D" doesn't conform to snake_case naming style (invalid-name)
load_xpcs_result.py:50:8: C0103: Variable name "ii" doesn't conform to snake_case naming style (invalid-name)
load_xpcs_result.py:51:12: C0103: Variable name "jj" doesn't conform to snake_case naming style (invalid-name)
load_xpcs_result.py:53:12: C0103: Variable name "ax" doesn't conform to snake_case naming style (invalid-name)
load_xpcs_result.py:79:4: C0103: Variable name "QZ_colormap" doesn't conform to snake_case naming style (invalid-name)
load_xpcs_result.py:82:9: C0103: Variable name "ax" doesn't conform to snake_case naming style (invalid-name)
load_xpcs_result.py:83:4: C0103: Variable name "im" doesn't conform to snake_case naming style (invalid-name)
load_xpcs_result.py:99:10: C0103: Variable name "ax" doesn't conform to snake_case naming style (invalid-name)

-------------------------------------------------------------------
Your code has been rated at 7.89/10 (previous run: 10.00/10, -2.11)

(py37) jemian@wow ~/.../projects/xpcs-nexus-8idi $ pylint --good-names "Int_2D, f, Iq, t0, g2, ii, jj, ax, im, QZ_colormap" load_xpcs_result.py 

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

That's about as good as I can hope for.

@alexchandel
Copy link

--good-names is insufficient because pylint is frequently invoked on large collections of files (including package files) by IDEs. A setting to disable the minimum length of 3 is needed, and there's clearly enormous demand for this.

@dcsanx
Copy link

dcsanx commented Jun 19, 2020

there are some names in the standard library that are camelCase eg like maxDiff
is the workaround to add these to 'good-names` ?

@Pierre-Sassoulas Pierre-Sassoulas added the Needs PR This issue is accepted, sufficiently specified and now needs an implementation label Jul 4, 2022
@helltraitor
Copy link

Can I add one more case?

@dataclass
class Track(DownloadItem):
    """
    Track class represents a track and its album.

    Track class implements DownloadItem protocol, so it can be downloaded.
    """
    album: Album
    id_: Id
    info: dict[str, Any] = field(default_factory=dict)
    meta: dict[str, Any] = field(default_factory=dict)

    # v- This part is highlighted in PyCharm (I'm using standard external tool - Pylint (latest version, on Python 3.10.5))
    ###
    def id(self) -> int:
        """
        Retrieves id from Id object.

        Returns:
            int: The id value of track.
        """
        return self.id_.value

This is very strange to get such kind of warnings. If you check conform criterion by length that you should exclude all short words

@clavedeluna
Copy link
Contributor

@Pierre-Sassoulas I'd be happy to help here since it's high priority, but given all the back and forth I can't figure out what the decision is here? What's the ask that would close this issue?

@Pierre-Sassoulas
Copy link
Member

Pierre-Sassoulas commented Nov 21, 2022

The short answer is that we need to create a new message name-too-short (and name-too-long?) and refactor the way the current regex works. The latest discussion about specification is on this issue : #7305. I think the proposal in #7305 is pretty convincing, but would probably be easier if we wait for a major and do it as a breaking changes. With 3.0 approaching, the perfect time to push for it would be from now to the time when we release 3.0. Feel free to refurbish the discussion on the other issue. (We're not closing this one because it's one of the most up-voted issue in the repo and closing it would just hide that.)

@Avasam
Copy link

Avasam commented Nov 21, 2022

In the mean time, if you use Flake8, you can try out https://github.com/PyCQA/pep8-naming . Personally it fit my needs better than pylint for naming convention. Since I tend to follow PEP8 naming conventions anyway.

@dotkrnl
Copy link

dotkrnl commented Apr 19, 2023

(Not affiliated with pylint, only providing a hint for people like me looking for workarounds)
For anyone who also wants to workaround this, you may consider using good-names-rgxs = "^..?$"

@jacobtylerwalls
Copy link
Member

jacobtylerwalls commented Jul 2, 2023

I have a proposal I'd appreciate feedback on. I think we need a fast fix for this that does not require thinking through a new spec as discussed in #7305.

Why?

  • Hiding a "short variable name" check inside the "name case" check is a major weakness for pylint, to the point that people are saying it's the reason they don't use the tool.
  • Even if we put a band-aid on it by describing it correctly in the output message ("hey the name is too short!"), we'd be fixing the wrong problem, since we should be decoupling casing from length.
  • This has been stuck for years.

In 3.0, if we just remove the length requirements from the default regexes, and explain in the release notes how to regain a check for length if wanted (via existing regexes), that would be good enough.

#2018 is a blocker for 3.0, I think. We can always do #7305 later. There is likely not time for significant new features in 3.0; we need to move to beta this summer.

Is there an easy way for users to regain the 3+ character requirement if we change the default regexes?
Yes!

  • If users are okay with disallowed-name instead of invalid-name, they can pass bad-names-rgxs=^..?$. That's short enough to suggest in the release note, and we can describe how it works, briefly. It would be more readable with an expression using {1,2}, but we'll need to fix bad-names-rgxs mangles regular expressions with commas #7229 first. (Good thing there's a PR: Ignore quantifiers when splitting comma-separated regexes #7241).
  • Or, if users really want --invalid-name emitted, the users can copy the existing regexes from the old source into their configs. We can also show that in the release notes, or even spell out the old regexes.

Either way, for the user that wants pylint to continue to fail names like x0, it's not a half-a-day upgrade, and we can describe how to do it. It puts the burden of configuration in the smaller user group (people who want a min name length feature even though pylint nowhere advertises such a feature, and merely has one accidentally). And compared to some of the other "off by default" changes we make all the time, like moving while-used to an extension, it's moving in the right direction in terms of getting pylint less opinionated.

@jacobtylerwalls jacobtylerwalls added this to the 3.0.0b1 milestone Jul 2, 2023
@DanielNoord
Copy link
Collaborator

I have a proposal I'd appreciate feedback on. I think we need a fast fix for this that does not require thinking through a new spec as discussed in #7305.

Why?

  • Hiding a "short variable name" check inside the "name case" check is a major weakness for pylint, to the point that people are saying it's the reason they don't use the tool.
  • Even if we put a band-aid on it by describing it correctly in the output message ("hey the name is too short!"), we'd be fixing the wrong problem, since we should be decoupling casing from length.
  • This has been stuck for years.

In 3.0, if we just remove the length requirements from the default regexes, and explain in the release notes how to regain a check for length if wanted (via existing regexes), that would be good enough.

#2018 is a blocker for 3.0, I think. We can always do #7305 later. There is likely not time for significant new features in 3.0; we need to move to beta this summer.

Is there an easy way for users to regain the 3+ character requirement if we change the default regexes? Yes!

  • If users are okay with disallowed-name instead of invalid-name, they can pass bad-names-rgxs=^..?$. That's short enough to suggest in the release note, and we can describe how it works, briefly. It would be more readable with an expression using {1,2}, but we'll need to fix bad-names-rgxs mangles regular expressions with commas #7229 first. (Good thing there's a PR: Ignore quantifiers when splitting comma-separated regexes #7241).
  • Or, if users really want --invalid-name emitted, the users can copy the existing regexes from the old source into their configs. We can also show that in the release notes, or even spell out the old regexes.

Either way, for the user that wants pylint to continue to fail names like x0, it's not a half-a-day upgrade, and we can describe how to do it. It puts the burden of configuration in the smaller user group (people who want a min name length feature even though pylint nowhere advertises such a feature, and merely has one accidentally). And compared to some of the other "off by default" changes we make all the time, like moving while-used to an extension, it's moving in the right direction in terms of getting pylint less opinionated.

Sounds good to me! Are you also working on a PR, I'd be happy to help with review. I think (like you said) this can be put clearly in the changelog, but we might also want to put this info a little bit more explicitly in the documentation. Can just be in the details.rst of the message as that is likely where people will end up looking for this.

@jacobtylerwalls
Copy link
Member

I'll put up a PR very shortly

@jacobtylerwalls jacobtylerwalls removed the Help wanted 🙏 Outside help would be appreciated, good for new contributors label Jul 2, 2023
@jacobtylerwalls jacobtylerwalls self-assigned this Jul 2, 2023
@jacobtylerwalls jacobtylerwalls modified the milestones: 3.0.0b1, 3.0.0a7 Jul 2, 2023
@dreamalligator
Copy link

amazing 👏

matkoniecz added a commit to matkoniecz/quick-beautiful that referenced this issue Dec 10, 2023
matkoniecz added a commit to osm-quality/wikibrain that referenced this issue Dec 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 🪲 Hacktoberfest High priority Issue with more than 10 reactions Needs PR This issue is accepted, sufficiently specified and now needs an implementation
Projects
None yet