Use predefined sets of naming styles instead of explicit regexps #1046

rogalski · 2016-07-23T19:17:06Z

An idea mentioned in issue #1013.

I am aware it's definitely not ready for merge, but I'd leave it here just to allow idea to sink and to hear feedback from you guys.

There are additional steps required for this change to be possibly merged:

Implement functionality of predefined presets (while keeping possibility of custom overrides by something_rgx options)
Select coding style presets to be included (possible candidates: snake_case (default), camelCase, PascalCase)
Define regular expressions for selected presets (I did not paid much attention to it in CamelCaseStyle)
Extend test suite for enough coverage

It may be a good idea to run a change on some big code bases with selected coding styles just to make sure we have covered all corner cases. We should aim for no false-positives (even if some false-negatives occurs), especially given that Pylint 2.0 is supposed to be a lot less nitpicking.

rogalski · 2016-07-23T19:40:40Z

FYI, Travis fails are two misaligned brackets from pylint report and pypy fail, which passes on my local machine. Likely it's an environment issue.

PCManticore · 2016-07-23T19:43:53Z

Hi @rogalski

This is really nice, thank you! I will do a thorough review tomorrow. You can ignore the Travis failure for now, at least the one from PyPy ( I didn't have time to investigate it yet).

rogalski · 2016-07-23T19:52:06Z

pylint/checkers/base.py

+            if overriden_value is not None:
+                regexps[name_type] = overriden_value
+
+            hints[name_type] = regexps[name_type].pattern


If we define naming convention presets in code, maybe we can provide better hint than pattern. Maybe hint could be a callable with single argument, where argument is a bad name provided by user. Now we can have non-static hint and suggest that serviceURL constant for snake_case should be renamed to SERVICE_URL.

Yeah, that would be interesting. So basically instead of

``Invalid name service`

we could have

Invalid name service, use SERVICE instead?

Yes, something like that. Just another step of making Pylint more user friendly (another feature similar to #1035 - don't tell me it's wrong, tell me how to fix it).

I'm aware that in general case it's quite hard and will be some kind of heuristic algorithm , but at the end, we can run heuristic result against predefined regex, and if it won't match just do not include suggestion in message.

Sounds much better than these static hints that are available currently.

PCManticore · 2016-07-26T16:37:16Z

pylint/checkers/base.py

        name_type = name_type.replace('_', '-')
        name_options.append((
            '%s-rgx' % (name_type,),
-            {'default': rgx, 'type': 'regexp', 'metavar': '<regexp>',


Why do we have to change the defaults for these?

Because these defaults would override every regular expressions taken from preset. Without removing these defaults, naming-style has no effect at all (condition in line 1285 is always True) .

Now, when --generate-rcfile is used, naming-style returns user friendly name, and all of custom regular expressions are included as placeholders (commented, without assigned value). Seems fair enough for for both normal users and power users.

Sample :

# Naming style used across a project (snake_case, camel_case). naming-style=snake_case # Regular expression matching correct module names #module-rgx= # Naming hint for module names #module-name-hint= # Regular expression matching correct constant names #const-rgx= # Naming hint for constant names #const-name-hint=

But this is different than what the linked issue suggested. We should have naming-styles separated of the regular expressions, one could take precendence over the other though. For instance, if I want to customize how my variables should look like, with a custom pattern, how would I do it now?

naming-style=snake_case variable-rgx=MY_NON_STANDARD_REGEX

All but variables are validated based on snake_case template, variables are validated based on MY_NON_STANDARD_REGEX.

rogalski · 2016-07-26T17:28:39Z

Oh, I re-read both issues and I guess I misunderstood it previously.
It suggested keeping individual naming styles for each type (module, variable, const etc.)
Here, there is one global naming style for project.

I think my approach is slightly better - how many projects reasonably require to have two naming conventions in same Python file?

PCManticore · 2016-07-26T17:43:54Z

That is not okay, we need to keep the same behavior for all the individual naming styles. For instance, you can have one naming conventions for classes and another one for methods, in case you are working with frameworks where they used camelCase convention previously (unittest, some packages from stdlib as well).

Ideally this patch should add just the naming style conventions, which can be used in turn of regexes when users want to, but it should not replace regexes per se. I am still looking into it and trying it right now.

rogalski · 2016-07-26T18:01:36Z

OK, thanks for review. I can give a shot for this ideal scenario, it should not be a big deal.

_I am still looking into it and trying it right now.

Can you elaborate? I'll stop working on it if you write exact same functionality right now.

PCManticore · 2016-07-26T18:14:15Z

No, no, sorry for being confusing. I'm looking into this pull request, testing it. I'm not planning to write anything. :-)

rogalski · 2016-12-04T19:10:46Z

@PCManticore I want to clean up on my dangling PR - is this usable in any form or should be just thrown away?

PCManticore · 2016-12-06T07:32:57Z

I think this is a good start, but haven't been able to follow up with a more detailed analysis on how this change should be. Let's leave opened for now and as soon as I make some time, I will come up with a proper review and change plan. If you close it, there's a high chance of me forgetting about this if time passes, so being opened makes me aware of the proposed change.

PCManticore · 2017-04-13T08:25:45Z

@rogalski I managed to review this a bit today. It looks nice! What I think it needs now is a release note in What's new and maybe a bit some more tests to make sure we are covering all the known cases. Feel free to do this at your own pace, since we will probably merge it after 1.7.0.

# Conflicts: # pylint/checkers/base.py

rogalski · 2017-08-13T19:20:21Z

Why xxx-naming-hint is config value to pylint? I'd expect that it's autogenerated message based on other config values (those that actually affects how pylint checks naming convention).

…ed on config

rogalski · 2017-08-13T20:22:00Z

@PCManticore let me know what you think about differences between 660b2d9 and 184b58a. I think both xxx-naming-style and include-naming-hint may be removed with this new model of predefined name styles.

PCManticore

@rogalski You mean name-hint and include-naming-hint, right? :) As mentioned in the review comments, I'm fine if name-hint goes away, but I would keep include-naming-hint for naming styles, since it is the only method of seeing how a pattern is supposed to look like, other than looking over the code of course.

PCManticore · 2017-08-14T08:53:30Z

pylint/checkers/base.py

+
+    @classmethod
+    def get_regex(cls, node_type):
+        return {


No need to build the dict every time this gets called, let's maybe create it once.

PCManticore · 2017-08-14T08:53:44Z

pylint/checkers/base.py

@@ -37,14  37,78 @@
 from pylint import reporters
 from pylint.reporters.ureports import nodes as reporter_nodes

+class NamingStyle(object):
+    """It may seem counterintuitive that single naming style


This looks more like a comment than a docstring

PCManticore · 2017-08-14T09:07:10Z

pylint/checkers/base.py

-        if self.config.include_naming_hint:
-            hint_rgx = getattr(self.config, hint_name.replace('-', '_'))
-            hint_part  = ' (hint: %r)' % hint_rgx
+        type_label = HUMAN_READABLE_TYPES[node_type]


I would still keep the naming hint flag (include-naming-hint) but only for naming styles, since the pattern already covers this. I'm thinking this would be a bit more helpful since there is no other way to see what we decided for a style to contain other than looking over the code (and the values won't be in the configuration either), so --include-naming-hint would be an user's only chance of seeing what a style value represents.

Have you seen emitted message from current top of this PR?

For out of the box settings:

Function name "BADFUNCTION_name" doesn't conform to snake_case naming style_

For user defined settings:

_Class name "classb" doesn't conform to '(?:(?P<UP>[A-Z] )|(?P<down>[a-z] ))$' pattern_

I personally like them a lot, and to reintroduce include-naming-hint, I'd effectively replace this verbose and precise definitions with something arbitrary like doesn't conform to selected naming style and doesn't conform to custom user-defined regular expression. Personally, I strongly prefer current form.

Include-naming-hint seems like a workaround for older messages where we emitted "Invalid class name 'classb'". With this new approach, where we provide both predefined and advanced mode for users, I don't see much value in keeping it.

Yes, I saw how it looks. Even if we show snake_case in the message, how an user will see what that means exactly without looking over the actual code, since these values are hardcoded in the code itself? I disagree about it being a workaround, especially since it's going to work for naming styles only

PCManticore · 2017-08-14T09:09:21Z

pylint/checkers/base.py

+
+            regexps[name_type] = NAMING_STYLES[naming_style_name].get_regex(name_type)
+
+            custom_regex_setting_name = "%s_rgx" % (name_type, )


There is a problem with picking the rgx if it exists, because currently we will always pick regexes, due to already having the -rgx hints pre-generated in config files. One idea would be to rename the options to something else, so for attr-rgx for instance, we should have attr-pattern or something along these lines. Could you think of another approach of how we should handle this?

currently we will always pick regexes, due to already having the -rgx hints pre-generated in config files

Yes, and I think it's fine, and should be kept this way. By doing it we won't introduce any regressions to current clients. On the same time, new users will happily use this basic functionality.

We simply need reasonable documentation of new capabilities, both in changelog / what's new and in "naming convention" section in docs.

Alright lets add documentation about this behaviour then

- AnyStyle - automated tests for generated regexes - reintroduce include-naming-hint

PCManticore · 2017-09-15T16:04:48Z

@rogalski This looks super nice, thank you! I just noticed you still have a couple of TODOs left in the tests, do you plan to fix those? Otherwise let's get this merged :)

rogalski · 2017-09-15T20:16:47Z

Hey Claudiu,
I'd take a last look at it somewhere in next week, clean that up (at least remove those todo comments) and merge it afterwards.

Thanks for approval!

rogalski · 2017-09-20T17:32:44Z

Merged, thanks for all valuable input!

Use predefined sets of naming styles instead of explicit regexps

b549a9b

rogalski reviewed Jul 23, 2016
View reviewed changes

fix linter issues

cc82fd5

PCManticore reviewed Jul 26, 2016
View reviewed changes

Create separate naming-style parameters instead of one global parameter

3abed5d

PCManticore added the Blocker 🙅 Blocks the next release label Sep 2, 2016

rogalski mentioned this pull request Jan 18, 2017

Spurious warning when using non-ASCII characters in function names #1176

Closed

PCManticore added Work in progress Needs review 🔍 Needs to be reviewed by one or multiple more persons and removed Blocker 🙅 Blocks the next release labels Mar 1, 2017

PCManticore added reviewed-waiting-updates and removed Needs review 🔍 Needs to be reviewed by one or multiple more persons labels Apr 13, 2017

rogalski added 4 commits August 13, 2017 20:25

Merge remote-tracking branch 'PyCQA/master' into name-presets

4c0a595

# Conflicts: # pylint/checkers/base.py

remove unused variables

8d603ec

tweak some issues introduced during merge

8cc8539

fix up all problems

b951e76

rogalski added 2 commits August 13, 2017 21:27

fix expected messages for tests

660b2d9

delete x-name-hint as pylint parameter, generate it automatically bas…

184b58a

…ed on config

rogalski added the Discussion 🤔 label Aug 13, 2017

rogalski removed the reviewed-waiting-updates label Aug 13, 2017

PCManticore requested changes Aug 14, 2017

View reviewed changes

rogalski added 3 commits August 17, 2017 19:42

Merge remote-tracking branch 'PyCQA/master' into name-presets

2ac8a3a

Next patch

3bdb1d2

- AnyStyle - automated tests for generated regexes - reintroduce include-naming-hint

options - initial draft

02e6b8b

rogalski added Needs review 🔍 Needs to be reviewed by one or multiple more persons and removed Work in progress labels Aug 24, 2017

PCManticore approved these changes Sep 15, 2017

View reviewed changes

PCManticore removed Discussion 🤔 Needs review 🔍 Needs to be reviewed by one or multiple more persons labels Sep 15, 2017

rogalski added 3 commits September 20, 2017 18:52

final cleanup

6721755

Merge remote-tracking branch 'PyCQA/master' into name-presets

0588b42

fixup changelog and whats new

e175f8c

rogalski merged commit 5b72e7f into pylint-dev:master Sep 20, 2017

zspitz mentioned this pull request Nov 19, 2017

Naming-style options not available #1744

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use predefined sets of naming styles instead of explicit regexps #1046

Use predefined sets of naming styles instead of explicit regexps #1046

rogalski commented Jul 23, 2016 •

edited

Loading

rogalski commented Jul 23, 2016 •

edited

Loading

PCManticore commented Jul 23, 2016

rogalski Jul 23, 2016 •

edited

Loading

PCManticore Jul 26, 2016

rogalski Jul 26, 2016 •

edited

Loading

PCManticore Jul 26, 2016

rogalski Jul 26, 2016 •

edited

Loading

PCManticore Jul 26, 2016

rogalski Jul 26, 2016 •

edited

Loading

rogalski commented Jul 26, 2016 •

edited

Loading

PCManticore commented Jul 26, 2016

rogalski commented Jul 26, 2016

PCManticore commented Jul 26, 2016

rogalski commented Dec 4, 2016

PCManticore commented Dec 6, 2016

PCManticore commented Apr 13, 2017

rogalski commented Aug 13, 2017 •

edited

Loading

rogalski commented Aug 13, 2017 •

edited

Loading

PCManticore left a comment

PCManticore Aug 14, 2017

PCManticore Aug 14, 2017

PCManticore Aug 14, 2017

rogalski Aug 17, 2017

PCManticore Aug 17, 2017

PCManticore Aug 14, 2017

rogalski Aug 17, 2017

PCManticore Aug 17, 2017

PCManticore commented Sep 15, 2017

rogalski commented Sep 15, 2017

rogalski commented Sep 20, 2017


		regexps[name_type] = NAMING_STYLES[naming_style_name].get_regex(name_type)

		custom_regex_setting_name = "%s_rgx" % (name_type, )

Use predefined sets of naming styles instead of explicit regexps #1046

Use predefined sets of naming styles instead of explicit regexps #1046

Conversation

rogalski commented Jul 23, 2016 • edited Loading

rogalski commented Jul 23, 2016 • edited Loading

PCManticore commented Jul 23, 2016

rogalski Jul 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rogalski Jul 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rogalski Jul 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rogalski Jul 26, 2016 • edited Loading

Choose a reason for hiding this comment

rogalski commented Jul 26, 2016 • edited Loading

PCManticore commented Jul 26, 2016

rogalski commented Jul 26, 2016

PCManticore commented Jul 26, 2016

rogalski commented Dec 4, 2016

PCManticore commented Dec 6, 2016

PCManticore commented Apr 13, 2017

rogalski commented Aug 13, 2017 • edited Loading

rogalski commented Aug 13, 2017 • edited Loading

PCManticore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PCManticore commented Sep 15, 2017

rogalski commented Sep 15, 2017

rogalski commented Sep 20, 2017

rogalski commented Jul 23, 2016 •

edited

Loading

rogalski commented Jul 23, 2016 •

edited

Loading

rogalski Jul 23, 2016 •

edited

Loading

rogalski Jul 26, 2016 •

edited

Loading

rogalski Jul 26, 2016 •

edited

Loading

rogalski Jul 26, 2016 •

edited

Loading

rogalski commented Jul 26, 2016 •

edited

Loading

rogalski commented Aug 13, 2017 •

edited

Loading

rogalski commented Aug 13, 2017 •

edited

Loading