[per_dir_config] Moved option parsing outside of linter #1886
Conversation
There was a problem hiding this comment.
@AWhetter Looks pretty great! Thanks for the amazing work.
I did a pass through it, the changes seems pretty straightforward, so I left only a couple of nitpicks comments. Looking forward to have this whole refactoring merged in the mainline.
pylint/config.py
Outdated
| config = Config() | ||
| config.add_options(self._option_definitions) | ||
| config = Configuration() | ||
| config.add_options(six.iteritems(self._option_definitions)) |
There was a problem hiding this comment.
Feel free to not use six.iteritems. When this patch will land, we will definitely be on Python 3+ only.
pylint/config.py
Outdated
| @@ -1014,8 +585,17 @@ def parse(self, file_path, config): | |||
| section = section.upper() | |||
|
|
|||
| for option, value in self._parser.items(section): | |||
| if isinstance(value, six.string_types): | |||
| @@ -216,58 +212,8 @@ def _patch_sysmodules(): | |||
| } | |||
|
|
|||
|
|
|||
| if multiprocessing is not None: | |||
There was a problem hiding this comment.
We should add a new issue in the per dir config project, if we don't have one already, for adding this back after we consolidate the changes we need.
There was a problem hiding this comment.
pylint/utils.py
Outdated
| """ | ||
| imported = {} | ||
| imported = set(('__init__', '__pycache__')) |
| continue | ||
| except ImportError as exc: | ||
| print("Problem importing module %s: %s" % (filename, exc), | ||
| file=sys.stderr) | ||
| else: | ||
| if hasattr(module, 'register'): | ||
| module.register(linter) | ||
| imported[base] = 1 | ||
| module.register(registry) |
There was a problem hiding this comment.
just a note for the future, this will be incompatible with every current pylint plugin out there. Before shipping all these changes, we should have documentation in place on how to update to say pylint 2.0/3.0 (whenever this is going to land) and maybe we should also add some runtime errors that inform the users why their plugin stopped working.
There was a problem hiding this comment.
Good idea! I've added a task to the project for this: https://github.com/PyCQA/pylint/projects/1#card-7678953
|
|
||
| option_groups = ( | ||
| ('Messages control', 'Options controlling analysis messages'), | ||
| ('Reports', 'Options related to output formatting and reporting'), | ||
| ) | ||
|
|
||
| def __init__(self, options=(), reporter=None, option_groups=(), | ||
| pylintrc=None): | ||
| def __init__(self, config=None): |
There was a problem hiding this comment.
This one looks so much better
| @@ -510,7 +420,7 @@ def _load_reporter(self): | |||
| self.set_reporter(reporter_class()) | |||
|
|
|||
| def _load_reporter_class(self): | |||
| qname = self._reporter_name | |||
| qname = self.config.output_format | |||
There was a problem hiding this comment.
Is the reporter name equivalent with the format?
There was a problem hiding this comment.
Yes. Currently PyLinter.set_option does this assignment (https://github.com/PyCQA/pylint/blob/master/pylint/lint.py#L546).
pylint/lint.py
Outdated
| def guess_lint_path(args): | ||
| """Attempt to determine the file being linted from a list of arguments. | ||
|
|
||
| Args: |
There was a problem hiding this comment.
Minor thing, I noticed we are not consistent with the docstring styles, we either use Google's one or restructuredtext, we should probably stick to one or another.
pylint/lint.py
Outdated
| Yields: | ||
| BaseChecker: Each registered checker. | ||
| """ | ||
| for checkers in six.itervalues(self._checkers): |
There was a problem hiding this comment.
Ditto regarding Python 3, this could be:
for checkers in self._checkers:
yield from checkers
This removed the OptionsManagerMixin and OptionsProvider classes, meaning that the ConfigParser classes now handle the option parsing and the Runner organises these parsers.
New Tasks to do:
Closes #972