-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Lint that META.yml files exist #11734
base: master
Are you sure you want to change the base?
Conversation
@@ -264,6 +264,8 @@ def name_is_non_test(self): | |||
self.name_prefix("MANIFEST") or | |||
self.filename == "META.yml" or | |||
self.filename.startswith(".") or | |||
self.filename == "OWNERS" or |
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.
When is this value necessary?
@@ -264,6 +264,8 @@ def name_is_non_test(self): | |||
self.name_prefix("MANIFEST") or | |||
self.filename == "META.yml" or | |||
self.filename.startswith(".") or | |||
self.filename == "OWNERS" or | |||
self.filename == "META.yml" or |
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.
This value is already included in the expression
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.
Hah, I never noticed that when I reviewed this before. I suck.
meta_yml_extensions_pattern = re.compile(b"^[a-zA-z0-9_]+:$") | ||
meta_yml_item_pattern = re.compile(b"^ - [a-zA-Z0-9-]+$") | ||
|
||
def check_meta_yml_contents(repo_root, path, contents): |
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 don't like this part much. I think we should use an actual yml parser to parse the files, and shouldn't require that all directories have suggested reviewers.
# dirs represents whether there's a META.yml file, value is a boolean. | ||
dirs = {} | ||
errors = [] | ||
reviewer_in_subdirs = [ |
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.
So I don't think this should mention reviewers
but the concept of requiring at least one META.yml
per independent part of spec seems sound.
Fixes #10552. Previously landed in #11092, reverted due to #11722.
TODO: The failure that caused #11722 is the same underlying issue as #9171: as an optimisation, we only pass changed files to lint, which doesn't suffice for the things that need to consider more than one file.