Remove the concept of checker priority #6034
Conversation
DanielNoord
added
the
Maintenance
Pull Request Test Coverage Report for Build 2063319443
💛 - Coveralls |
No I can't access the bitbucket issue, so I can't see a discussion in the original PR. After working on this some more, I feel like a lot of the @jacobtylerwalls Thoughts? |
| # Sort checkers by priority | ||
| needed_checkers = sorted( | ||
| needed_checkers, key=operator.attrgetter("priority"), reverse=True | ||
| ) |
There was a problem hiding this comment.
Is anyone aware of pylint ever having a failfast mode? That's all I can think of for the usefulness of sorting checkers. If you want the more important checkers to run first when failing fast.
There was a problem hiding this comment.
I don't think we have a failfast mode but you're right it's probably a use case. We could want to order by warnings significance instead of checkers later (i.e. to raise most significant messages first) see #5604. Cutting other checkers analysis if we don't have messages to show in them would be nice performance wise, but I don't think we need priority to do that.
There was a problem hiding this comment.
We don't have a failfast mode, but this wasn't really documented or implemented, so we can always put one back if we want it and document it.
| assert provider.priority <= 0, "provider's priority can't be >= 0" | ||
| for i, options_provider in enumerate(self.options_providers): | ||
| if provider.priority > options_provider.priority: | ||
| self.options_providers.insert(i, provider) | ||
| break | ||
| else: | ||
| self.options_providers.append(provider) | ||
| self.options_providers.append(provider) |
There was a problem hiding this comment.
Option provider priority is different than checker priority, right? We should document that as going away also.
I'm a little nervous that a behavior change could be hiding here, but maybe you can reassure me :D. I tried to trace downstream from this where this could be used and noticed ConfigurationMixin (subclass of OptionsManagerMixIn) is used in pyreverse, which is way outside my involvement.
Also, apparently checkers are option providers? So could removing the checker priority alter the status quo for what order checker-provided-options get overwritten? But if this is part of removing complexity during the argparse migration that's fine...
There was a problem hiding this comment.
Well, since each check is a option provider they share the same priority. Thus, we're basically doing the same checks here as we're doing at that other place.
I think the only two "functions" that come from these priorities is that:
- Checkers will be run sooner, but when is that ever needed?
- Options could be overwritten by providers with higher priority. But then again, why would we allow that? That creates a lot of mess and duplicate options have actually caused me multiple headaches before.
There was a problem hiding this comment.
That creates a lot of mess and duplicate options have actually caused me multiple headaches before.
+1 for reducing the API footprint by removing features we don't understand / aren't documented while migrating to argparse anyway!
doc/whatsnew/<current release.rst>.and preferred name in
script/.contributors_aliases.jsonType of Changes
Description
I don't really see what this does? There is this issue #229.
But it seems more like a "let's make a function that uses something that is already there" than something that actually adds something. When is it ever useful to have some checker be executed 50ms earlier than another?
I'd vote for removal. I don't consider this a breaking change because you can still define
prioritywithout it breaking anything.And we can easily revert this commit if something does depend on it.