-
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
Issue_58 - Add support for conditions #59
base: main
Are you sure you want to change the base?
Conversation
Hey @Levi-Leah - could you review this one? This PR adds supports for ifdef and ifndef conditions when parsing global attributes and coalescing files. It also addresses the following edge cases -
|
renaming a pcmd command
Update README.md
Issue_66: Added yaml_validation before preview runs.
PantheonCMD/pcbuild.py
Outdated
for condition in conditions_list: | ||
if not condition in attributes.keys(): | ||
conditions_missing = True | ||
else: | ||
conditions_missing = True | ||
if conditions_missing: | ||
conditions_missing = False |
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.
same here, both if and else statements result in conditions_missing = True
. can't it be:
for condition in conditions_list: | |
if not condition in attributes.keys(): | |
conditions_missing = True | |
else: | |
conditions_missing = True | |
if conditions_missing: | |
conditions_missing = False | |
if conditions_missing: | |
conditions_missing = False | |
else: | |
conditions_missing = True |
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.
Hmm - yep, good point.
I'll run through this one again to see if there was something I was trying to do there. Maybe I've just confused myself.
PantheonCMD/pcbuild.py
Outdated
elif not conditions in attributes.keys(): | ||
conditions_missing = True |
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 the above is applied this part might also be redundant
PantheonCMD/pcbuild.py
Outdated
if matches.group(2).strip() != '': | ||
lines.append(matches.group(2).strip() + '\n') |
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 matches.group(2).strip() != '': | |
lines.append(matches.group(2).strip() + '\n') | |
lines.append(matches.group(2).strip() + '\n') |
PantheonCMD/pcbuild.py
Outdated
for condition in conditions_list: | ||
if condition in attributes.keys(): | ||
conditions_missing = True | ||
else: | ||
conditions_missing = True | ||
if conditions_missing: | ||
conditions_missing = False | ||
|
||
# Single condition | ||
elif conditions in attributes.keys(): | ||
conditions_missing = True |
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.
similar to one of my comments above:
for condition in conditions_list: | |
if condition in attributes.keys(): | |
conditions_missing = True | |
else: | |
conditions_missing = True | |
if conditions_missing: | |
conditions_missing = False | |
# Single condition | |
elif conditions in attributes.keys(): | |
conditions_missing = True | |
if conditions_missing: | |
conditions_missing = False | |
else: | |
conditions_missing = True |
PantheonCMD/pcbuild.py
Outdated
if matches.group(2).strip() != '': | ||
lines.append(matches.group(2).strip() + '\n') |
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 matches.group(2).strip() != '': | |
lines.append(matches.group(2).strip() + '\n') | |
lines.append(matches.group(2).strip() + '\n') |
@adahms |
Simplified conditions block end logic. Co-authored-by: Levi <[email protected]>
Simplified conditions block end logic. Co-authored-by: Levi <[email protected]>
Hey @Levi-Leah - finally returning to this one. I was definitely tying myself up in knots a bit there - I've accepted several of your suggestions and re-worked the logic so that it's a little easier to read and parse. Let me know what you think if you've got the time for a review. :) |
I'll try to finish the review this week |
No description provided.