Skip to content

Figure out a better way of registering options? #2355

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

Closed
AWhetter opened this issue Jul 28, 2018 · 5 comments
Closed

Figure out a better way of registering options? #2355

AWhetter opened this issue Jul 28, 2018 · 5 comments
Labels
High effort 🏋 Difficult solution or problem to solve Per directory config New per directory config feature
Milestone

Comments

@AWhetter
Copy link
Contributor

All of the config parsers (the CLIParser and the IniFileParser) need the option definitions and so does the Configuration. Registering a checker means registering options, which means the PluginRegistry needs a callback function to give these options to all of these objects.

@Pierre-Sassoulas
Copy link
Member

I would not mind for the processing of option to be handled by argparse or even better confuse or click in a simple manner. Optparse is deprecated since python 3.2. Even if Optparse was state of the art, the Mixin classes are really confusing for me. I think it's something we need to keep in mind : it's probably easier to nuke the whole thing and start fresh with a newer library than doing it little refactor of Optparse by little refactor of Optparse.

The checker could also use composition instead of inheritance in order to get their options. It would break the coupling we have right now between option parsing and code analysis. Changing the way option work is made very difficult because of this coupling. Test are using the checker by giving option to Optparse, so the refactor of option registering is huge because of that if you change the option registering you need to change the checker tests too (!).

So I think it might be necessary to first make the BaseChecker an independent base class before actually doing anything to make options registering better.

@DanielNoord
Copy link
Collaborator

@Pierre-Sassoulas Is there a reason why options are given to BaseChecker? Can pylint be ran without instantiating a PyLinter class?

To me it seems much more logical to add all options to PyLinter at once and allowing all checkers to access them from self.linter or from a util function that gets them from the PyLinter class. This would mean that we would move all current optdicts to a separate file/module which gets loaded once by PyLinter on initialisation.

Problems to consider would be:

  1. What is the performance effect of loading unnecessary options (ie, options from disabled checkers/extensions)
    I think this is marginal compared to the double loading we often do now
  2. How can extensions or checkers add options to the PyLinter.options attribute?
  3. How do organize the main list of optdicts?

@Pierre-Sassoulas
Copy link
Member

I think it's theorically possible to launch a single checker but in practice we never do that and always use pylint through the pylinter. Might not be true for plugins ?

@DanielNoord
Copy link
Collaborator

Yeah, that's what I was wondering. Do we have an "plugin expert" who would know this?

@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.14.0 milestone Apr 5, 2022
@DanielNoord
Copy link
Collaborator

@Pierre-Sassoulas I think we can close this? There is still #5829, but I think that deserves its own issue and doesn't necessarily fall under this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
High effort 🏋 Difficult solution or problem to solve Per directory config New per directory config feature
Projects
None yet
Development

No branches or pull requests

3 participants