Skip to content

Conversation

mndeveci
Copy link
Contributor

@mndeveci mndeveci commented Jun 6, 2022

Issue #, if available:
N/A

Description of changes:
Checks if method is not None before proceeding to work on it.

Description of how you validated changes:
Adding a new test case and verifying the older ones.

Checklist:

  • Add/update unit tests using:
  • Add/update integration tests
  • make pr passes
  • Update documentation
  • Verify transformed template deploys and application functions as expected
  • Do these changes include any template validations?
    • Did the newly validated properties support intrinsics prior to adding the validations? (If unsure, please review Intrinsic Functions before proceeding).
      • Does the pull request ensure that intrinsics remain functional with the new validations?

Examples?

Please reach out in the comments, if you want to add an example. Examples will be
added to sam init through https://github.com/awslabs/aws-sam-cli-app-templates/

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@mndeveci mndeveci merged commit 5046f68 into aws:develop Jun 6, 2022
@mndeveci mndeveci deleted the check_null_method branch June 6, 2022 21:02
Copy link
Contributor

@torresxb1 torresxb1 left a comment

Choose a reason for hiding this comment

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

I see this for method_definition in self.get_conditional_contents(method) is used a few times in the file. In order to catch this early without having to run this validation each time, could we add this check in the constructor?
We do something like this in the SwaggerEditor constructor: https://github.com/aws/serverless-application-model/blob/develop/samtranslator/swagger/swagger.py#L65

By the way, could we also add this check on the SwaggerEditor 🙏

@torresxb1
Copy link
Contributor

oh, I was too late

@mndeveci
Copy link
Contributor Author

mndeveci commented Jun 6, 2022

oh, I was too late

No worries, let me raise a new PR to address your comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants