Skip to content
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

expand search for docs and schema.yml (#1832) #2058

Merged
merged 2 commits into from
Jan 22, 2020

Conversation

beckjake
Copy link
Contributor

Fixes #1832

This is based on the PR #2051 - merge that first!

  • update tests
  • Turn configs into dataclasses to remove a bunch of repetitive junk
  • appease mypy

update tests
appease mypy
@cla-bot cla-bot bot added the cla:yes label Jan 17, 2020
Copy link
Contributor

@drewbanin drewbanin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of comments here, mostly for my edification, but this LGTM!

if not (isinstance(other, self.__class__) and
isinstance(self, other.__class__)):
return False
return NotImplemented
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to return NotImplemented here, or should this raise an internal exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's up with this line? Were we not supplying the cli_vars before? Or is this also to appease mypy?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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__.

@drewbanin
Copy link
Contributor

ship it!

@beckjake beckjake merged commit 5c42f1a into feature/docs-for-everyone Jan 22, 2020
@beckjake beckjake deleted the feature/expand-schema-search branch January 22, 2020 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants