Skip to content

Use main instead of master as main checker name #6569

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

Merged
merged 4 commits into from
May 10, 2022

Conversation

DanielNoord
Copy link
Collaborator

  • 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>.
  • Write a good description on what the PR does.
  • 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
✨ New feature
🔨 Refactoring
📜 Docs

Description

Closes #5467.

Sorry for the size of the PR, but I just went through all cases of the term master.

I think it would be good to do this in the beta as well as this could unexpectedly break stuff and is then very easy to revert.

@DanielNoord DanielNoord added Enhancement ✨ Improvement to a component Documentation 📗 Maintenance Discussion or action around maintaining pylint or the dev workflow labels May 10, 2022
@DanielNoord DanielNoord added this to the 2.14.0 milestone May 10, 2022
@coveralls
Copy link

coveralls commented May 10, 2022

Pull Request Test Coverage Report for Build 2299752502

  • 2 of 2 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 95.326%

Totals Coverage Status
Change from base Build 2299467367: 0.0%
Covered Lines: 16029
Relevant Lines: 16815

💛 - Coveralls

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'd feel safer if we handled both i.e. we keep the old MASTER in the tests, and add one test using MAIN.

@DanielNoord
Copy link
Collaborator Author

I'd feel safer if we handled both i.e. we keep the old MASTER in the tests, and add one test using MAIN.

We have tests for MASTER already. See:
https://github.com/PyCQA/pylint/tree/main/tests/config/functional/setup_cfg/deprecate_master

That's why I am not too worried about changing this across our own code. We support both since our migration to argparse so there is no reason to keep a (for some people) loaded term in. Let's change while we're taking the time to look into this anyway.

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.

The argparse migration permits to do a lot of thing easily, that was the main component of 2.14 by far imo. Great work @DanielNoord !

@DanielNoord
Copy link
Collaborator Author

I'm just worried about any potential regressions... 😅

Until now only bad-option-value gave real headaches, but we're so deep in now that it will be difficult to revert stuff that inadvertently broke things. For example, the incompatibility with colorama is quite annoying. I tried to get them to release a new patch version but haven't succeeded so far.

Let's hope the beta helps spot issues before we release them to production.

@DanielNoord DanielNoord merged commit 7c91521 into pylint-dev:main May 10, 2022
@DanielNoord DanielNoord deleted the main-master branch May 10, 2022 09:38
@Pierre-Sassoulas
Copy link
Member

Until now only bad-option-value gave real headaches

I'm a little worried about bad-option-value being raised if a check became an extension (no-self-use). What do you think about handling this use case ? We're going to get this a lot because I can see how a lot of pylintrc would disable it right now.

@DanielNoord
Copy link
Collaborator Author

Until now only bad-option-value gave real headaches

I'm a little worried about bad-option-value being raised if a check became an extension (no-self-use). What do you think about handling this use case ? We're going to get this a lot because I can see how a lot of pylintrc would disable it right now.

I think that's a feature not a bug. It immediately tells people to either add the extension to their config if they want to keep using it or remove it from their disabled list of they no longer want to.

bad-option-value has some difficult trade-offs, but I think in the long run its effect on keeping pylint configurations up to date will have a positive effect on the pylint ecosystem.

@Pierre-Sassoulas
Copy link
Member

Say you already explicitly disabled no-self-use, what's the value brought by having to remove it because it became optional in pylint ? It just feel like double the work (and just imagine if we want to change our mind about including a check, they'd need to re-disable it). I would be very conservative in forcing users to clean up their config file manually. How about a warning for explicit enable telling the user what plugin need to be loaded now and nothing for explicit disable ?

@DanielNoord
Copy link
Collaborator Author

In that first case there is no specific value other than a shorter config file.

The only change users who don't want to receive these messages ever again is add bad-option-value once to the top of their disable list in their config file if they upgrade to 2.14+. That should prevent any warnings from popping up about this ever again. (That could be considered a breaking change, but...)

Adding anything else to this imo just unnecessarily complicates things. It's a 1 or 2 minute change after pre-commit makes a PR against your branch to update to the new version. I think such a change isn't too much to ask for when upgrading a dependency. The benefits and downsides of the added complexity just don't fully weigh up for me: the group of users that will be pissed off by this vs. the group that welcomes us more pro-actively warning about this + the increased maintainability of simpler code.

@Pierre-Sassoulas
Copy link
Member

Wouldn't a basic version of the auto-migration in #5462 (doable by just loading a conf and re-generating it immediately) fix this issue by making it automated though ?

@DanielNoord
Copy link
Collaborator Author

You would still need a manually maintained mapping of moved messages and extensions as well finding a way to determine whether a bad-option-value is due to a typo, a moved message, a removed message, a missing ,, etc.

It might not be as simple as removing any bad-option-value we encounter.

@Pierre-Sassoulas Pierre-Sassoulas removed this from the 2.14.0 milestone May 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation 📗 Enhancement ✨ Improvement to a component Maintenance Discussion or action around maintaining pylint or the dev workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Updating [MASTER] terminology to [MAIN] while preserving backwards compatibility
3 participants