Test for astroid not being broken by new pylint checks

[Pierre-Sassoulas]

Is your feature request related to a problem? Please describe

When we release a new check in pylint it can break astroid's tox checks because a new problem is detected in astroid.

Describe the solution you'd like

Add a check in pylint's CI so the person making the new check in pylint have to fix astroid too. It would smooth astroid fixing process and it would add an example and be an acceptance tests of the new check.

But it would mean we need to download astroid during pylint's tests.

Additional context

pylint-dev/astroid#968 (Following the creation of consider-using-with in #4372)

[Pierre-Sassoulas]

@cdce8p what do you think ?

[cdce8p]

I'm not sure it's quite that easy. Sure we could add the CI test and I do agree some kind of acceptance test would be useful. However, the tradeoff is that a developer who just to fix some small bug would also need to fix astroid which he/she probably doesn't know anything about. There is also the case of rare circular dependencies: We need to fix astroid to modify pylint and astroid needs the modified pylint version for its tests.

Regarding astroid, the best thing I can think of would probably be to run the pylint tests for every astroid MR. That way we could catch regression bugs caused by astroid more easily.

As for the acceptance test, I'm open for ideas. Running them would certainly be useful. For the last couple releases I have run the current master over the Home Assistant repo. That's how I caught #4380 that early for example. The drawback here is that with every new checker those test obviously can start to fail until the repo is update. So I don't know if they are that useful for acceptance tests. It might be wroth taking another look at how mypy and black are doing it with their xx-primer checks.

[Pierre-Sassoulas]

I created a ticket to separate acceptance tests vs astroid/pylint new checks breaking astroid.

Regarding astroid, it can be a short fix, just adding the new check to the ignore list in pylintrc. The alternative is the maintainers having to do the upgrade themself from time to time and it does not make much more sense. Yes it's something more to do but for example in the case of consider-using-with it would have forced the implementer to think about what happen if the intent of the code was to create a function that will be used in a with ? Should the warning now become : "this would make more sense in a function called __enter__" ?

[cdce8p]

just adding the new check to the ignore list in pylintrc.

If it's just that, we can probably do it ourselves.

The alternative is the maintainers having to do the upgrade themself from time to time and it does not make much more sense.

Why not? True, it's an additional burden for the maintainers but honestly we are the ones who know the codebase way better. If we ask contributors to open a MR for astroid that disables their new checker, why should we update pylint at all? The point, as I understand it, is to improve your own codebase with the help of pylint. From time to time, that would mean we would have to do pylint updates.

Yes it's something more to do but for example in the case of consider-using-with it would have forced the implementer to think about what happen if the intent of the code was to create a function that will be used in a with ? Should the warning now become : "this would make more sense in a function called __enter__" ?

I haven't followed it that closely to have an opinion. However from what I observed, it indeed finds some cases that can be rewritten. Cloud it be further improved, obviously, but that is besides the point.

In the end I'm just worrying that we place to high of a burden on especially new, inexperienced developers when they just what to contribute a bug fix or an idea for a new checker.

[Pierre-Sassoulas]

OK that make sense, let's not do that.