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

chore: add $or operator to the EventPattern interface #28368

Closed
wants to merge 1 commit into from

Conversation

b-dur
Copy link

@b-dur b-dur commented Dec 14, 2023

This PR adds a missing $or comparison operator to the EventPattern interface. As describe in the doc the $or is not limited to only be on the leaf-node.

Before this change the TS compiler would throw Object literal may only specify known properties, and '$or' does not exist in type 'EventPattern' Error. By adding // @ts-ignore one could get ride of the TS error and the code/rule would work as expected.

With this change the TS compiler no longer raises this error and $or is an excepted property on the EventPattern interface.

Integration test was not run.

It is uncertain what prefix should have been used for this PR. Is Fix. more accurate?


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

@github-actions github-actions bot added p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Dec 14, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team December 14, 2023 15:22
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

@b-dur b-dur changed the title add $or comparison to the EventPattern type fix: add $or operator to the EventPattern interface Dec 14, 2023
@b-dur b-dur changed the title fix: add $or operator to the EventPattern interface chore: add $or operator to the EventPattern interface Dec 14, 2023
@aws-cdk-automation aws-cdk-automation dismissed their stale review December 14, 2023 15:56

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

@b-dur b-dur force-pushed the eventpattern-missing-or-operator branch 3 times, most recently from 771e721 to f223017 Compare December 14, 2023 20:00
@b-dur
Copy link
Author

b-dur commented Dec 14, 2023

The error seeing now is:

 error JSII8002: Method and property (unless they are static readonly) names must use camelCase. Rename "aws-cdk-lib.aws_events.EventPattern.$or" to "or"

I wonder if there is a way to avoid this? Or can or be used instead of $or ?

@b-dur b-dur force-pushed the eventpattern-missing-or-operator branch from f223017 to 564a335 Compare December 15, 2023 08:26
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 564a335
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Copy link
Contributor

@kaizencc kaizencc 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 the docs that say "Comparison operators only work on leaf nodes, with the exception of $or and anything-but." so I'm willing to entertain this. This PR needs:

  • the property can't be named $or. This really means 'anyOf' right? Lets name it that
  • integ test demonstrating that we can use this property at the root level.

@aws-cdk-automation
Copy link
Collaborator

This PR has been in the BUILD FAILING state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

@aws-cdk-automation
Copy link
Collaborator

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

@aws-cdk-automation aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Jan 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants