-
Notifications
You must be signed in to change notification settings - Fork 5
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
validation through entry point + reorg of the arg space #68
base: main
Are you sure you want to change the base?
Conversation
@adahms FYI, I split the messaging into it's own file because this code kept repeating. It's a work in progress. |
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.
Hey @Levi-Leah - added a couple of comments for your consideration.
Overall, the function works as intended - the proposed changes are mostly of a philosophical nature.
For example, if we're validating an entry point, is it sufficient for the entry point to specify the attributes the included content requires, or should we still inject the main attributes file or files?
Some of the coalescing function might also come in handy for looping through content, but I understand the use case you've got in mind here is somewhat different from what that one's doing. :)
PantheonCMD/pcyamlchecks.py
Outdated
print("FAIL: there is an error in your yaml file:") | ||
for key in v.errors.keys(): | ||
print("\n\t'{}' {}".format(key, ', '.join(str(item) for item in v.errors[key]))) | ||
print("ERROR: there is an error in your yaml file:") |
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 this changes to 'ERROR', perhaps it makes more sense to phrase the message as 'Your YAML file contains an error.' or something similar.
PantheonCMD/pcentrypointvalidator.py
Outdated
|
||
def get_concatenated_includes(entry_point_list): | ||
existing_entry_points = get_exist(entry_point_list) | ||
level_one_includes = get_level_one_includes(existing_entry_points) |
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.
These functions are acting as wrappers for a single command today.
If you'd like to keep this format, perhaps this chain can be simplified to something similar to the following -
level_one_includes = get_includes(existing_entry_points)
level_two_includes = get_includes(level_one_includes)
...
PantheonCMD/pcentrypointvalidator.py
Outdated
sys.exit(2) | ||
|
||
|
||
def get_includes(entry_points): |
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.
You might run into a similar problem I encountered with the coalescing function here.
Namely, some users use attributes in their includes - and nested ones at that. As such, unless you go through and resolve the entire attribute tree, attempting to convert an include into an actual path sometimes fails.
How would you feel about a pared down version of the coalescing loop to still work with attributes and iterate through levels of content?
The #60 pull request is getting too big for review so I cherry-pickked the new functionality into this branch.
With
pcmd validate --e file/path/entry-point.adoc
command you can validate files from an entry point recursive.It's mesnt for master.adoc validation but can validate any assembly as easily.