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.

Sign up for GitHub

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

Jump to bottom

Add statement condition class #46

Merged
merged 29 commits into from
May 4, 2021
Merged

Add statement condition class #46

merged 29 commits into from
May 4, 2021

Conversation

oscarbc96
Copy link
Contributor

@oscarbc96 oscarbc96 commented Apr 19, 2020 •
edited
Loading

Improvements

  • Add StatementCondition class, with a function resolver as a replacement for ConditionDict.

Removes

  • Removes constants CONDITION_MODIFIERS and CONDITION_FUNCTIONS from pycfmodel/constants.py
  • Removes is_conditional_dict from pycfmodel/utils.py

Fixes

  • Update CLOUDFORMATION_ACTIONS

ocrawford555
ocrawford555 reviewed Apr 20, 2020
View reviewed changes
Copy link
Contributor

@ocrawford555 ocrawford555 left a comment

Choose a reason for hiding this comment

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

left some small comments, but overall really nice work. Share some of Jordi's comments, about knowing if we have covered all cases or not, and logging when some unexpected function is seen :)

pycfmodel/utils.py Outdated Show resolved Hide resolved
pycfmodel/model/resources/properties/statement_condition.py Outdated Show resolved Hide resolved
pycfmodel/model/resources/properties/statement_condition.py Outdated Show resolved Hide resolved
pycfmodel/model/resources/properties/statement_condition.py Outdated Show resolved Hide resolved
@oscarbc96 oscarbc96 changed the base branch from add_isort to master April 20, 2020 13:29
oscarbc96 and others added 2 commits April 20, 2020 16:21
@oscarbc96 @jsoucheiron @ocrawford555
Apply suggestions from code review
cd5cb05 
Co-Authored-By: Jordi Soucheiron <jsoucheiron@users.noreply.github.com>
Co-Authored-By: Oliver Crawford <16978487+ocrawford555@users.noreply.github.com>
@oscarbc96
apply comments from PR review
de8cd6b
ocrawford555
ocrawford555 previously approved these changes Apr 20, 2020
View reviewed changes
Copy link
Contributor

@ocrawford555 ocrawford555 left a comment

Choose a reason for hiding this comment

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

Nice work 👍

@oscarbc96 oscarbc96 changed the title WIP: Add statement condition class Add statement condition class Apr 30, 2021
@oscarbc96
Copy link
Contributor Author

Tests are fixed in this PR: #57

@oscarbc96 oscarbc96 marked this pull request as ready for review May 3, 2021 13:42
jsoucheiron
jsoucheiron approved these changes May 4, 2021
View reviewed changes
Copy link
Member

@jsoucheiron jsoucheiron left a comment

Choose a reason for hiding this comment

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

LGTM, nice work

ocrawford555
ocrawford555 approved these changes May 4, 2021
View reviewed changes
Copy link
Contributor

@ocrawford555 ocrawford555 left a comment

Choose a reason for hiding this comment

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

Freaking awesome Oscar, LGTM ⚡ ⚡ ⚡

ignaciobolonio
ignaciobolonio approved these changes May 4, 2021
View reviewed changes
Copy link
Contributor

@ignaciobolonio ignaciobolonio left a comment

Choose a reason for hiding this comment

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

Nice job

@oscarbc96 oscarbc96 merged commit e6567f7 into master May 4, 2021
@ocrawford555 ocrawford555 deleted the add_statement_condition branch June 11, 2021 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsoucheiron jsoucheiron jsoucheiron approved these changes

@ocrawford555 ocrawford555 ocrawford555 approved these changes

@ignaciobolonio ignaciobolonio ignaciobolonio approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@oscarbc96 @jsoucheiron @ocrawford555 @ignaciobolonio