expand search for docs and schema.yml (#1832) #2058
Conversation
update tests appease mypy
| if not (isinstance(other, self.__class__) and | ||
| isinstance(self, other.__class__)): | ||
| return False | ||
| return NotImplemented |
There was a problem hiding this comment.
do we want to return NotImplemented here, or should this raise an internal exception?
There was a problem hiding this comment.
I think the canonical correct behavior is returning NotImplemented in this situation: https://docs.python.org/3/library/constants.html#NotImplemented
and that devolves into False if the types don't match.
| threads=threads, | ||
| credentials=credentials | ||
| ) | ||
| args: Any |
There was a problem hiding this comment.
i think you can remove (or at least move 😛) the comment above
| @@ -144,44 +150,61 @@ def _parse_versions(versions): | |||
| return [VersionSpecifier.from_version_string(v) for v in versions] | |||
|
|
|||
|
|
|||
| def _all_source_paths( | |||
There was a problem hiding this comment.
why enumerate the different resource paths here instead of just passing in a list of paths, or varargs, or something like that? I buy it, just curious about your reasoning
There was a problem hiding this comment.
My reasoning was that at some point in the future, we're going to have to update this (macros and analyses). Currently two different places call this function and the data should be in sync.
With this signature, either mypy/pylint will catch it or we'll crash at runtime with a line pointing to the call that's missing arguments during parsing. That will never accidentally get merged because no tests will ever pass. With a list of paths/varargs, it'll be easy to forget one place or the other and not notice.
There was a problem hiding this comment.
awesome, makes a ton of sense
| @@ -109,17 +73,18 @@ def from_parts(cls, project, profile, args): | |||
| config=profile.config, | |||
| threads=profile.threads, | |||
| credentials=profile.credentials, | |||
| args=args | |||
| args=args, | |||
| cli_vars=cli_vars, | |||
There was a problem hiding this comment.
What's up with this line? Were we not supplying the cli_vars before? Or is this also to appease mypy?
There was a problem hiding this comment.
Because they're all dataclasses the actual class does now take a cli_vars argument, and it isn't optional. Now it's the responsibility of classmethods instead of __init__. Before we set cli_vars in __init__.
|
ship it! |
Fixes #1832
This is based on the PR #2051 - merge that first!