-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add collection meta code #168
Add collection meta code #168
Conversation
removal: | ||
major_version: 11 | ||
reason: other | ||
reason_text: The collection wasn't cow friendly, so the Steering Committee decided to kick it out. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moo
collection_metadata: str, | ||
all_collections: list[str], | ||
expected_errors: list[str], | ||
tmp_path, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I usually annotate fixtures (and import Path from pathlib in this case), but that's up to you.
tmp_path, | |
tmp_path: Path, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if not os.path.exists(collection_meta_path): | ||
return [f"Cannot find {collection_meta_path}"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noticing this now: it would be more efficient if this was moved to the top of the function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally the path was only composed at this point, so it maked sense to have that test there. Since it's now passed in, it definitely makes sense!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co-authored-by: Maxwell G <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved!
Co-authored-by: Maxwell G <[email protected]>
@gotmax23 thanks for reviewing this! |
Follow-up to ansible-community/antsibull-build#617. Used by ansible-community/antsibull-build#620.