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

Compile regexes for @pattern strings early #2058

Merged
merged 29 commits into from
Jan 9, 2023
Merged

Conversation

jjant
Copy link
Contributor

@jjant jjant commented Dec 5, 2022

Description

Completes #2026.

  • Initializes regexes during ${service}Builder::build()
  • Code generates tests making sure the regexes compile in rust

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@jjant jjant force-pushed the jjant/init-regexes-early branch from f48be55 to 0edb00f Compare December 5, 2022 16:02
@jjant jjant marked this pull request as ready for review December 5, 2022 16:03
@jjant jjant requested a review from a team as a code owner December 5, 2022 16:03
@jjant jjant requested review from 82marbag and hlbarber December 5, 2022 16:03
@jjant jjant requested a review from a team as a code owner December 7, 2022 16:58
@jjant jjant requested a review from david-perez December 7, 2022 20:22
Copy link
Contributor

@david-perez david-perez left a comment

Choose a reason for hiding this comment

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

This is good but we should also code-generate a #[test] per pattern compiling each regex to do even earlier detection.

A Kotlin unit test exercising a model whose pattern is accepted by Smithy but not by the regex crate that asserts that the tests fail and that the service panics at startup would be nice too.

@jjant jjant requested a review from a team as a code owner December 16, 2022 10:50
@jjant jjant requested a review from david-perez December 20, 2022 10:27
@jdisanti jdisanti added the server Rust server SDK label Dec 20, 2022
@smithy-lang smithy-lang deleted a comment from github-actions bot Dec 21, 2022
@smithy-lang smithy-lang deleted a comment from github-actions bot Dec 21, 2022
@smithy-lang smithy-lang deleted a comment from github-actions bot Dec 21, 2022
…egen/server/smithy/generators/ConstrainedStringGenerator.kt

Co-authored-by: Luca Palmieri <[email protected]>
@smithy-lang smithy-lang deleted a comment from github-actions bot Jan 5, 2023
@smithy-lang smithy-lang deleted a comment from github-actions bot Jan 5, 2023
@jjant jjant requested a review from rcoh January 5, 2023 12:36
@github-actions
Copy link

github-actions bot commented Jan 5, 2023

A new generated diff is ready to view.

A new doc preview is ready to view.

@smithy-lang smithy-lang deleted a comment from github-actions bot Jan 5, 2023
@smithy-lang smithy-lang deleted a comment from github-actions bot Jan 5, 2023
@jjant jjant requested a review from david-perez January 5, 2023 14:40
@jjant
Copy link
Contributor Author

jjant commented Jan 5, 2023

@david-perez Added a Kotlin test in 2873529.

@github-actions
Copy link

github-actions bot commented Jan 6, 2023

A new generated diff is ready to view.

A new doc preview is ready to view.

Comment on lines +207 to +209
assertThrows<CommandFailed> {
project.compileAndTest()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we in any way verify the message it fails with, to make sure it fails for the reason we expect?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah—you can look at the error message. An example:

        val failure = shouldThrow<CommandFailed> { "cargo test".runWithWarnings(testDir) }
        failure.output shouldContain "endpoint::test::test_1"
        failure.output shouldContain "https://failingtest.com"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lovely, thanks!

Copy link
Collaborator

@rcoh rcoh left a comment

Choose a reason for hiding this comment

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

lgtm!

@github-actions
Copy link

github-actions bot commented Jan 9, 2023

A new generated diff is ready to view.

A new doc preview is ready to view.

@jjant jjant enabled auto-merge (squash) January 9, 2023 10:56
@github-actions
Copy link

github-actions bot commented Jan 9, 2023

A new generated diff is ready to view.

A new doc preview is ready to view.

@jjant jjant merged commit 76f0fa9 into main Jan 9, 2023
@jjant jjant deleted the jjant/init-regexes-early branch January 9, 2023 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
server Rust server SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants