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

Suppress OSError in config file discovery #7423

Merged
merged 2 commits into from Sep 6, 2022

Conversation

DanielNoord
Copy link
Collaborator

  • Write a good description on what the PR does.
  • Create a news fragment with towncrier create <IssueNumber>.<type> which will be
    included in the changelog. <type> can be one of: new_check, removed_check, extension,
    false_positive, false_negative, bugfix, other, internal. If necessary you can write
    details or offer examples on how the new change is supposed to work.
  • If you used multiple emails or multiple names when contributing, add your mails
    and preferred name in script/.contributors_aliases.json

Type of Changes

Type
🐛 Bug fix

Description

Closes #7169

@DanielNoord DanielNoord added the Configuration Related to configuration label Sep 6, 2022
@DanielNoord DanielNoord added this to the 2.15.1 milestone Sep 6, 2022
@coveralls
Copy link

coveralls commented Sep 6, 2022

Pull Request Test Coverage Report for Build 3000802112

  • 22 of 27 (81.48%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.006%) to 95.328%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pylint/config/find_default_config_files.py 22 27 81.48%
Totals Coverage Status
Change from base Build 2999709475: -0.006%
Covered Lines: 17017
Relevant Lines: 17851

💛 - Coveralls

@DanielNoord
Copy link
Collaborator Author

Coverage is same as on main.

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did'nt you say that t contextlib performance were worst than try/except ?

@DanielNoord
Copy link
Collaborator Author

Did'nt you say that t contextlib performance were worst than try/except ?

It is (marginally) I think.

But since this only called once and the code became really unreadable with all the try ... except everywhere I opted for this solution.

Want me to change?

@Pierre-Sassoulas
Copy link
Member

Want me to change?

Not at all, just wanted to know your opinion :)

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a feeling this is getting really complicated. Also we're not handling errors. Failing to yield a configuration file because of a permission error will make pylint not take a file into account for non obvious reasons so it seems like we need to do something on top of suppression the error outright.

How about we create some function with explicit name like "get all ini config file" or "search for configuration file in parent directories". That way we can launch a single function in the contextlib.suppresserror or use try/except and maybe decide later what to do easily in case of error.

@Pierre-Sassoulas
Copy link
Member

In the issue you're fixing right now the reason is PermissionError: [Errno 13] Permission denied: '/home/myhome/.config/pylintrc', we might want to have a message like "/home/myhome/.config/pylintrc found but is impossible to use because of: PermissionError: [Errno 13] Permission denied: '/home/myhome/.config/pylintrc'"

@DanielNoord
Copy link
Collaborator Author

I have a feeling this is getting really complicated. Also we're not handling errors. Failing to yield a configuration file because of a permission error will make pylint not take a file into account for non obvious reasons so it seems like we need to do something on top of suppression the error outright.

How about we create some function with explicit name like "get all ini config file" or "search for configuration file in parent directories". That way we can launch a single function in the contextlib.suppresserror or use try/except and maybe decide later what to do easily in case of error.

Yeah, I can refactor this into separate functions.

I can also print something to stdout when we are in verbose mode? I don't think warning every time is desired. In the original report this seems like it is a situation the usr can't do anything about. So warning every time would be annoying. Would printing in verbose mode be enough for you?

@Pierre-Sassoulas
Copy link
Member

I can also print something to stdout when we are in verbose mode?

Sounds good !

@github-actions

This comment has been minimized.

@DanielNoord
Copy link
Collaborator Author

I can also print something to stdout when we are in verbose mode?

Sounds good !

😅

Bit of a chicken and egg. How can we know if we are in verbose mode if we are still looking for a configuration to load...

I would be okay with not doing anything. I think the annoyance of warning about this if you already know about the issue outweighs the issue of silently ignoring.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2022

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit 3e67b1f

@Pierre-Sassoulas Pierre-Sassoulas added the Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer label Sep 6, 2022
@Pierre-Sassoulas Pierre-Sassoulas merged commit 8b46e22 into pylint-dev:main Sep 6, 2022
@DanielNoord DanielNoord deleted the rcfile branch September 6, 2022 20:03
@Pierre-Sassoulas Pierre-Sassoulas added Backported and removed Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer labels Sep 6, 2022
Pierre-Sassoulas pushed a commit to Pierre-Sassoulas/pylint that referenced this pull request Sep 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported Configuration Related to configuration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default rcfile locations are scanned even when --rcfile option is specified
3 participants