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

Remove safe_decode() and deprecated_option() from utils #5046

Merged

Conversation

DanielNoord
Copy link
Collaborator

  • Add yourself to CONTRIBUTORS if you are a new contributor.
  • Write a good description on what the PR does.

Type of Changes

Type
βœ“ πŸ”¨ Refactoring

Description

While working on the typing of pylint/utils/utils.py I noticed these two functions never get called. Therefore they seem like candidates for removal from the codebase.

@DanielNoord DanielNoord added the Maintenance Discussion or action around maintaining pylint or the dev workflow label Sep 20, 2021
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.

Too bad coverall is down, this would have been Γ  pleasant report :D thanks for the cleanup !

@Pierre-Sassoulas Pierre-Sassoulas merged commit dea0beb into pylint-dev:main Sep 20, 2021
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.11.2 milestone Sep 20, 2021
@DanielNoord DanielNoord deleted the remove-unused-functions branch September 20, 2021 07:29
@cdce8p
Copy link
Member

cdce8p commented Sep 20, 2021

Just a followup, for the future should we deprecate methods first before we remove them?

@Pierre-Sassoulas
Copy link
Member

Yeah theoretically, I just don't think of pylint.utils.utils as a public API so imo if it's not used in pylint own code it's not used at all. Maybe I'm wrong and we should have added it to the list of deprecation for 3.0.

@DanielNoord
Copy link
Collaborator Author

I was thinking the same as Pierre, but you are indeed right Marc. Sorry about this!

@cdce8p
Copy link
Member

cdce8p commented Sep 20, 2021

Yeah theoretically, I just don't think of pylint.utils.utils as a public API so imo if it's not used in pylint own code it's not used at all.

What about get_global_option? That is also part of pylint.utils.utils. I would consider this at the very least useful for plugins.

Maybe I'm wrong and we should have added it to the list of deprecation for 3.0.

Regarding the two methods, I do agree that a deprecation probably wasn't necessary.

--
What this makes clear however is that it might be time to have a discussion (or define) which parts of the API are considered public and which private. Especially with the ongoing refactoring effort, this is something to think about. Just as example the changes and deprecations in #5041.

@Pierre-Sassoulas
Copy link
Member

On the top of my head the public API is:

  • Package information (in pkginfo.py)
  • BaseChecker, DeprecationMixin (in pylint django at least)
  • pylint.interfaces
  • BaseReporter ?
  • pylint.testutil
  • Json output for IDE integration
  • text output that some tools must be parsing like it's always going to stay that way (including me when I pipe and filter the message to disable when approaching a new project).

I would need to check the major plugin that depends on pylint:

Now that I think of it, the "is it a public API" approach is the wrong one, we should probably ask ourselves what is not instead. And I would say "the small helper functions". The safest way would be to consider everything public API, but then we could never refactor anything without releasing a major and that would make our live really hard, right ?

@DanielNoord
Copy link
Collaborator Author

Just to round off this discussion, I think it will be hard to come up with a one solution/rule that fits all cases. We should probably be a little bit more considerate about what not to remove outright, but should not fully constrain ourselves. In the latest refactoring PRs I think we striked the right balance!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Discussion or action around maintaining pylint or the dev workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants