Strict mypy on sqlfluff.core
#6246
Conversation
Coverage Results ✅ |
| def get_rules(self) -> List[Type[RuleType]]: | ||
| """Get plugin rules. | ||
|
|
||
| NOTE: While the type annotation for this method returns `Type[RuleType]` | ||
| all plugin implementations should instead import `sqlfluff.core.rules` and | ||
| return the more specific `Type[BaseRule]`. This base definition of the | ||
| PluginSpec cannot import that yet as it would cause a circular import. | ||
| """ |
There was a problem hiding this comment.
@WittierDinosaur - this is the only bit that feels a bit gross, but this is the least bad of a few choices. Those options are:
- (what I've done, which is) Have the plugin module describe something "rule-like" which
get_rules()returns, and that be a parent class ofBaseRuleso that mypy is happy. - We could have the type annotation here reference
BaseRuledirectly, but that violates thelint-importsrule which says that thelintercan't depend onrules(it must be the other way around only) - and at the moment,linterdepends onconfigwhich depends onplugin.linteris likely always going to depend onconfig, and refactoringconfigso that it doesn't need to depend onpluginseems hard or even impossible. - We could avoid the new
RuleTypeclass and just make this type hint really vague (i.e.List[Any]orList[Type[Any]]). You could argue that the currentRuleTypeimplementation isn't much better than this because the class doesn't actually define anything other than being a marker for inheritance, but it feels a little more descriptive than this.
What do you reckon?
There was a problem hiding this comment.
for context, the problem we're trying to solve is:
What should the type annotation on
get_rules()be?
Strict mypy wanted it to have something more specific than just list.
There was a problem hiding this comment.
Ignore all of this. I think it's gross. I've just added an exception in the import linter instead and imported BaseRule for TYPE_CHECKING.
| # TODO: This type annotation could probably be more specific but that would | ||
| # require making the config info object something more like a namedTuple rather | ||
| # than a dict. | ||
| def get_configs_info(self) -> Dict[str, Dict[str, Any]]: |
There was a problem hiding this comment.
I'm not proposing that I resolve this TODO in this PR. It's more of a marker for later improvements. What I've done here is already better than what was here before.
This follows on from #6240 and should support #6220.
This completes the job of making the whole of
sqlfluff.corecompliant withmypy --strict.The majority of this is fairly uncontroversial and just fiddly type annotations. The only two more major changes are
A slight hack for theUPDATE: Ignore this, I think it's gross so just added an exception in the import linter to allow it. There's already two other exceptions in there for type checking.hookspec, which involved moving thetypesmodule up a level, and introducing aRuleTypeclass as a base class for rules, but which only acts as a return type for the baseget_rules()method.get_contextfor the templaters, including a change for the jinja templater which use to override this method, and now implements it's own custom method which calls this method to allow it to change the type signature.I've also added a bit more documentation around the
py.typedfile.