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

Add support for per-directory configuration #5833

Closed
wants to merge 5 commits into from

Conversation

matusvalo
Copy link
Collaborator

@matusvalo matusvalo commented Feb 23, 2022

Type of Changes

Type
πŸ› Bug fix
βœ“ ✨ New feature
πŸ”¨ Refactoring
πŸ“œ Docs

Description

This simple implementation of per-directory configuration file is done by moving discovery code outside of PyLinter class. During executing of pylint, list of modules/packages to be analysed is created and for each of the module/package is discovered "closest" configuration file - hence mapping of configuration file and list of packages/modules covered by configuration file is created. Then separate linter is created for each configuration file.

NOTE: This implementation is more PoC. I would like to kindly ask for review of approach and feedback. Also discussion about logic of per-directory configuration files is needed (e.g. should we introduce inheritance of conf. files and how - some discussion is already present in #618)

PROS:

  • simple implementation, no big breaking changes needed to be introduced

CONS:

  • some overhead can be introduced
  • thread management is per linter which can be suboptimal, better is carve out worker threads out of PyLinter class

TODO:

  • Statistics are printed for each linter separately. They should be merged into one
  • Linters loads only config file of FS subtree. We do not support oThis should be discussed. Maybe we should introduce overriding of conf. file.
  • overhead can be checked and some caching introduced to avoid repetitive scanning of FS subtree
  • tests needs to be added
  • Add a ChangeLog entry describing what your PR does.
  • If it's a new feature, or an important bug fix, add a What's New entry in
    doc/whatsnew/<current release.rst>.

Closes #618

@DanielNoord
Copy link
Collaborator

@matusvalo Thanks for taking the time to look into this. I haven't looked at the code at all, but:

I'd like to wait with doing this until the transition to argparse is complete. This is likely going to change how we handle configuration internally and we can probably make it easier to add this. You can take a look at #5392 and #5584 for some of the related work towards this.
It's currently halted by the release of 2.13 but I intend to go full steam ahead afterwards πŸ˜„

@matusvalo
Copy link
Collaborator Author

matusvalo commented Feb 23, 2022

I have looked in PR #5584 and the changes are in different levels. This PR is changing mainly interaction with pylint.lint.run.Run class which is transparent against argument handling (arguments are handled in deeper layers), this PR is just moving around raw arguments. But I am not sure what changes will be required for rest of open points (but argument handling I suppose not)

EDIT: I read the commend second time and I saw that you are going to change handling of configuration file - yes this can affect this PR definitely. But in any case, I think it would be beneficial to review this PR, maybe it will help with your changes and I am willing to update this PR based on your changes elsewhere.

@DanielNoord
Copy link
Collaborator

I have looked in PR #5584 and the changes are in different levels. This PR is changing mainly interaction with pylint.lint.run.Run class which is transparent against argument handling (arguments are handled in deeper layers), this PR is just moving around raw arguments. But I am not sure what changes will be required for rest of open points (but argument handling I suppose not)

EDIT: I read the commend second time and I saw that you are going to change handling of configuration file - yes this can affect this PR definitely. But in any case, I think it would be beneficial to review this PR, maybe it will help with your changes and I am willing to update this PR based on your changes elsewhere.

The problem is that I'm not really sure how we're going to change things. In 2.13 we're changing some internals that handle multiprocessing which are a pre-requisite for changing the configuration handling.
So only after that release I can actually start working on the full change and see what problems and issues I run into. There is so much going on with the configuration handling that I'm really not sure what the final design will look like atm. Would it be okay for you to let this PR rest for a bit, give me some time to work on argparse and then revisit?

@matusvalo
Copy link
Collaborator Author

Would it be okay for you to let this PR rest for a bit, give me some time to work on argparse and then revisit?

No problem take your time. Ping me when I can proceed with this. Till then I will put this on hold.

@DanielNoord
Copy link
Collaborator

@matusvalo I have complete most of the transition to argparse so if you want I think you can pick this up again. I haven't analysed your approach completely, but I think initialising a new linter class for every module might have too much of an performance impact.

What about storing the modules, closest configuration and a namespace object for that configuration? We would only need to reparse the configuration file and command line for each config file and then before we lint a file set the correct namespace object to config. Instead of juggling around PyLinter we would be juggling around argparse.Namespaces. I think that would be more efficient. Let me know what you think!

@matusvalo
Copy link
Collaborator Author

matusvalo commented Apr 17, 2022

I haven't analysed your approach completely, but I think initialising a new linter class for every module might have too much of an performance impact.

If I remember the code correctly, the PR is creating new linter not for every module but only for every rcfile. I don't suppose that project will have more than several rc files - e.g. one for the code base and one for unittests. Hence for all current project having just one configuration file, the overhead is 0 except the need of scanning the FS tree for files which we need to do anyway.

What about storing the modules, closest configuration and a namespace object for that configuration? We would only need to reparse the configuration file and command line for each config file and then before we lint a file set the correct namespace object to config.

This is also possibility. I need to merge main, review changes and determine how complex it would be to implement.

@DanielNoord
Copy link
Collaborator

@matusvalo I'm not sure if you're still interested in this, but I have done some preliminary work here:
#618 (comment)

I think some of the changes in this PR can be helpful for that approach, but we just need to extract them into their own PRs.

Let me know if you're still interested and want me to help you do so!

@DanielNoord DanielNoord added the Waiting on author Indicate that maintainers are waiting for a message of the author label Jun 21, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jun 3, 2023

This PR needs take over because because it has been open 8 weeks with no activity.

@github-actions github-actions bot added the Needs take over πŸ›ŽοΈ Orignal implementer went away but the code could be salvaged. label Jun 3, 2023
@DanielNoord
Copy link
Collaborator

Closing this as it has become outdated. Thanks for the inspiration though matus!

@DanielNoord DanielNoord closed this Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion πŸ€” Needs take over πŸ›ŽοΈ Orignal implementer went away but the code could be salvaged. Waiting on author Indicate that maintainers are waiting for a message of the author Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add different configuration for different sub directories
3 participants