Fix 972 argparse #1596

AWhetter · 2017-07-27T03:49:04Z

This is part of the per directory config work (#618) so this is being merged into per_dir_config and not master. It also isn't 100% production ready.
Fixing #972 and moving the command line parsing out of the PyLinter class is a huge change. So I'm splitting it up into smaller chunks. Although, this has still ended up being pretty big so I'll explain some of the changes here. Don't feel as though you can't make statements about the overall design, and feel free to be picky even though this is a big change.

This first chunk of work removes optparse in favour of argparse. I've also added the start of some classes that we'll eventually start using once we fully move the config parsing out of PyLinter:

Configuration: This is an argparse.Namespace like object that will store all of the configuration values and be supplied to each checker through the self.config variable of the checker. At the moment is mostly provides an abstraction from argparse so that we aren't giving each checker an argparse.Namespace. Eventually it will also be useful for merging configs which we'll need to implement include directives in configs.
ConfigurationStore: We don't use this at the moment but eventually it'll be used for keeping track of what config objects belong to what directory. Currently it's merging configs from parent directories just to give an example of how we can merge Configuration objects but eventually it will check the config for include directives and merge as appropriate (unless it ends up making more sense for the file parser to do it).
ConfigParser: This is a base class for anything that reads a configuration from somewhere. It was a bit undecided whether this needed to be a class because generally they're small, but I think keeping things inheriting from ConfigParser will make it easier for others to create a new ConfigParser classes to get a config from other sources (eg a gui, registry) without also having to write a whole Runner class.
CLIParser: At the moment this is pretty much just a container for an argparse.ArgumentParser but eventually I'm hoping that we'll only need to call preprocess and parse. To make wedging this into optik a bit easier the optik code is using the argparse.ArgumentParser directly but the end goal is for outside users to need to interact with the class through OptionDefinition objects only.
IniFileParser: Like CLIParse this is mostly a container for an OptionParser at the moment and will be more useful later.
LongHelpFormatter and LongHelpArgumentParser: These are objects to implement --long-help, --long-long-help, etc in argparse. argparse doesn't let you do much to override it's help formatting behaviour so I'm having to do a bit of fudgery to get this to work. Mostly this is setting the level on the formatter class because there's too much argparse machinery to override the methods that call the formatter class to call the formatter class methods with a level. This was the least hacky way I could see of doing things without relying on argparse implementation details. Let me know if you can think of another way!

Breaking changes:

csv type arguments can no longer have a space after the comma.
Option callbacks are no longer called with optparse arguments, but with argparse type arguments instead. For example value is no longer an optparse.Option but an argparse.Action.
For now I've broken manpage generation but I need to have a think about how we'll implement this in the future. I probably won't use a HelpFormatter like we did with optparse but instead convert the OptionDefinition objects to the manpage directly.

The next step for #972 is to remove option callbacks altogether. This will also mean we stop dispatching options to OptionProvider objects and instead set options on the Configuration objects directly.

Also added unused method for finding a nearby pylintrc for when we turn on per directory configs.

PCManticore

Thank you for this PR, Ashley! And sorry for taking so long to review it.
Looks really nice so far and seems to be a great step towards replacing optparse and making the config situation in pylint more sane. Left a bunch of comments for an initial review round, let's keep it open for now until you address the remainder of the stuff (TODOs and whatnot(

PCManticore · 2017-08-14T08:13:01Z

pylint/config.py

+    search_dir = os.path.expanduser(search_dir)
+    path = find_pylintrc_in(search_dir)
+
+    for search_dir in walk_up(search_dir):


Let's check the path condition before going in the loop, something as in:

if not path: for search_dir in walk_up(search_dir): if not os.path.isfile(...): break ... if path: ...

Done in an upcoming commit!

PCManticore · 2017-08-14T08:14:43Z

pylint/config.py

+    path = find_pylintrc_in(search_dir)
+
+    for search_dir in walk_up(search_dir):
+        if path or not os.path.isfile(os.path.join(search_dir, '__init__.py')):


How will this work for Python 3.3+ namespace packages? From my understanding, it seems this change won't support them. I'm wondering if we should though, what is your opinion on it?

You're right, it won't. We could do something like; once we don't find an __init__.py, keep looking for the closest pylintrc file and only use it if a find a setup.py in a parent directory. I'm not sure how I feel about looking for setup.py files because it's possible that the package isn't installed that way, but I can't think of another way of doing it.

Yeah, I feel the same way about a setup.py. Maybe expose a limit of levels, e.g. lookup maximum 5 levels, if there is no pylintrc in any of those, bail out with an exception?

We'll have a global/commond line config by this point so that should work!

PCManticore · 2017-08-14T08:18:11Z

pylint/config.py

-        yield
-    finally:
-        optparse.HelpFormatter.expand_default = orig_default
+            and option.help is not argparse.SUPPRESS]


(commenting here because I don't have other useful context) Won't we support any longer the various option kinds that we have now yn, regexp_csv, multiple_choices? Or the plan is to add support for them after this PR? Sorry if you mentioned this already, I can't seem to find any reference to these in this PR.

We can still support these. In _convert_definition I'm converting any existing validators from a string (eg "yn", "regexp_csv") to their validator functions so that argparse can use it.

PCManticore · 2017-08-14T08:31:51Z

pylint/config.py

+
+    @staticmethod
+    def _convert_definition(option, definition):
+        """Convert an option definition to a set of arguments for add_argument.


Let's maybe pick a style and stick with it? :) This file seems to have both Google and Sphinx style.

Good point. Work habits sneaking in ;) I've done everything to sphinx.

PCManticore · 2017-08-14T08:39:58Z

pylint/config.py

        # add provider's specific options
        for opt, optdict in options:
            self.add_optik_option(provider, group, opt, optdict)

    def add_optik_option(self, provider, optikcontainer, opt, optdict):
        args, optdict = self.optik_option(provider, opt, optdict)
-        option = optikcontainer.add_option(*args, **optdict)
+        if hasattr(optikcontainer, '_parser'):
+            optikcontainer = optikcontainer._parser


I hope we can get rid of this optik names later on.

Me too. I'll be phasing it out as more and more of this config logic disappears.

PCManticore · 2017-08-14T08:43:08Z

pylint/config.py

+    def add_option(self, option_definition):
+        name, definition = option_definition
+        if name in self._option_definitions:
+            # TODO: Raise something more sensible


Yup, let's do that

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

pylint/config.py

+            definition (dict): The argument definition to convert.
+
+        Returns:
+            tuple(str, list, dict): A tuple of the group to add the argument to,


Maybe return a namedtuple?

Normally I would agree but this is a private method used in only one place, so it seems like overkill. I could make it a private namedtuple though if you'd prefer to have it?

Yup, let's leave it as is!

AWhetter · 2017-09-09T20:08:26Z

Looks like it's taken me just as long to get back to you ;) Thanks for having a look. The rest of the TODOs are things to be fixed in future PRs, before this gets merged into master.

PCManticore · 2017-09-30T10:02:55Z

Looks good @AWhetter !

AWhetter added 7 commits July 9, 2017 23:38

Split up find_pylintrc

9075109

Also added unused method for finding a nearby pylintrc for when we turn on per directory configs.

Slotted in IniFileParser and Configuration

2b3b86b

Slotted in argparse with CLIParser

7f6c898

Implemented long help with argparse

56de1f3

Fixed callback options needing to take an argument

b56ab4b

Fixed option callbacks not getting run

481e768

Finished removing optparse

1ef7fd3

AWhetter added the Per directory config New per directory config feature label Jul 27, 2017

AWhetter requested a review from PCManticore July 27, 2017 03:49

Fixed Python2 syntax errors

0a51ec2

PCManticore reviewed Aug 14, 2017

View reviewed changes

AWhetter added 4 commits September 9, 2017 20:23

fixup! Split up find_pylintrc

75d8961

Moved walk_up to utils

e41efee

Started using more specific exceptions in config

eb488dd

Fixed up docstrings

49b2c77

AWhetter force-pushed the fix_972_argparse branch from e5b7905 to 49b2c77 Compare September 9, 2017 20:06

PCManticore approved these changes Sep 30, 2017

View reviewed changes

AWhetter merged commit 8640fa4 into pylint-dev:per_dir_config Oct 8, 2017

AWhetter deleted the fix_972_argparse branch October 8, 2017 21:24

Uh oh!

Fix 972 argparse #1596

Fix 972 argparse #1596

Uh oh!

Conversation

AWhetter commented Jul 27, 2017

Uh oh!

PCManticore left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AWhetter commented Sep 9, 2017

Uh oh!

PCManticore commented Sep 30, 2017

Uh oh!

Uh oh!