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

Issue_58 - Add support for conditions #59

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Issue_58 - Add support for conditions #59

wants to merge 16 commits into from

Conversation

adahms
Copy link
Collaborator

@adahms adahms commented Nov 2, 2021

No description provided.

@adahms adahms added the enhancement New feature or request label Nov 2, 2021
@adahms adahms self-assigned this Nov 2, 2021
@adahms adahms linked an issue Nov 2, 2021 that may be closed by this pull request
@adahms adahms changed the title WIP: Issue_58 - Initial conditional parsing logic. WIP: Issue_58 - Add support for conditions Nov 8, 2021
@adahms adahms changed the title WIP: Issue_58 - Add support for conditions Issue_58 - Add support for conditions Nov 8, 2021
@adahms adahms requested a review from Levi-Leah November 8, 2021 03:00
@adahms
Copy link
Collaborator Author

adahms commented Nov 8, 2021

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 -

  • Merged the global attribute parsing function into the coalescing function so that the global attributes file is treated like any other modular documentation file. This has the added benefit of compiling attributes from includes in the global attributes file into a single dictionary, as in the RHDG documentation.
  • Updates line tests to use the same regex format, and tests for attributes in includes that aren't at the start of the include.

PantheonCMD/pcbuild.py Outdated Show resolved Hide resolved
Comment on lines 158 to 164
for condition in conditions_list:
if not condition in attributes.keys():
conditions_missing = True
else:
conditions_missing = True
if conditions_missing:
conditions_missing = False
Copy link
Collaborator

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:

Suggested change
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

Copy link
Collaborator Author

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.

Comment on lines 167 to 168
elif not conditions in attributes.keys():
conditions_missing = True
Copy link
Collaborator

@Levi-Leah Levi-Leah Nov 30, 2021

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

Comment on lines 174 to 175
if matches.group(2).strip() != '':
lines.append(matches.group(2).strip() + '\n')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if matches.group(2).strip() != '':
lines.append(matches.group(2).strip() + '\n')
lines.append(matches.group(2).strip() + '\n')

Comment on lines 198 to 208
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
Copy link
Collaborator

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:

Suggested change
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

Comment on lines 214 to 215
if matches.group(2).strip() != '':
lines.append(matches.group(2).strip() + '\n')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if matches.group(2).strip() != '':
lines.append(matches.group(2).strip() + '\n')
lines.append(matches.group(2).strip() + '\n')

@Levi-Leah
Copy link
Collaborator

@adahms
I added a few suggestions regarding the simplification of some if statements, please take them with a grain of salt

@adahms
Copy link
Collaborator Author

adahms commented Mar 8, 2022

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. :)

@Levi-Leah
Copy link
Collaborator

I'll try to finish the review this week

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for conditionals
3 participants