Skip to content

Conversation

MEPalma
Copy link
Contributor

@MEPalma MEPalma commented Dec 13, 2024

Motivation

Currently the SFN internpreter v2 represents string literals though a keyword_or_string sub-production or through specialised terminal token types. This approach lacked an intermediary representation for string carrying executable expressions (jsonpaths, context objects, intrinsic functions, jsonata expressions, variable sampling), leading to some logic duplication as each declaration often having to implement the evaluation logic of these strings in varying context. These changes introduce the string_expression sub-production and a few logic child productions. Most importantly, all its sub productions are preprocessed as the expected evaluation component for the specific string expression they represent. The result is a more delcarative grammar, which ensures the correct string expressions are placed in the intended productions, with the productions seemingly having to just delegate the string expression provided, or apply the relevant logic overrides.

Changes

  • introduced the string_expression concept
  • introduced reusable string expression evaluation component
  • updated the grammar to be more delcarative in the expected string expressions for each sub-production
  • applied relevant changes for this migration

@MEPalma MEPalma added the semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases label Dec 13, 2024
@MEPalma MEPalma added this to the 4.1 milestone Dec 13, 2024
@MEPalma MEPalma self-assigned this Dec 13, 2024
Copy link

github-actions bot commented Dec 13, 2024

LocalStack Community integration with Pro

    2 files  ±    0      2 suites  ±0   32m 17s ⏱️ - 1h 19m 59s
1 333 tests  - 2 568  1 269 ✅  - 2 325  64 💤  - 243  0 ❌ ±0 
1 335 runs   - 2 568  1 269 ✅  - 2 325  66 💤  - 243  0 ❌ ±0 

Results for commit 28bcfff. ± Comparison against base commit a25d0d9.

This pull request removes 2568 tests.
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_lambda_dynamodb
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_opensearch_crud
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_search_books
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_setup
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_destination_sns
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_infra
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_prefill_dynamodb_table
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input0-SUCCEEDED]
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input1-SUCCEEDED]
…

♻️ This comment has been updated with latest results.

Copy link
Contributor

@gregfurman gregfurman 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 questions. Happy to approve when those comments are removed 😄

Also, very happy with this approach!

@MEPalma MEPalma requested a review from gregfurman December 18, 2024 11:02
Copy link
Contributor

@gregfurman gregfurman left a comment

Choose a reason for hiding this comment

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

LGTM!

@MEPalma MEPalma merged commit c76cc60 into master Dec 18, 2024
31 checks passed
@MEPalma MEPalma deleted the MEP-sfn-string_expr branch December 18, 2024 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants