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

Allowing for trailing dollars and percent signs in String literals #4625

Merged
merged 7 commits into from
Oct 29, 2024

Conversation

mccartney
Copy link
Contributor

@mccartney mccartney commented Oct 29, 2024

What's changed?

Changing the HCL lexing/parsing grammar to allow for $ and % signs at the end of String literals.

What's your motivation?

Until now it hasn't been allowed, which I think is a bug as described in:

Checklist

  • I've added unit tests to cover both positive and negative cases (some already exist)

@timtebeek timtebeek self-requested a review October 29, 2024 14:44
@timtebeek timtebeek added bug Something isn't working parser-hcl labels Oct 29, 2024
@mccartney
Copy link
Contributor Author

mccartney commented Oct 29, 2024

Hmm. Now I see the autogenerated Java files by ANTLR observe removed copyright/license notices. Not sure how to restore them (other than manual crafting them). Looking into it.

@timtebeek
Copy link
Contributor

./gradlew licenseFormat

ought to help; although the bot also should chime in soon.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Some suggestions could not be made:

  • rewrite-hcl/src/main/java/org/openrewrite/hcl/internal/grammar/HCLParserBaseListener.java
    • lines 1-0
  • rewrite-hcl/src/main/java/org/openrewrite/hcl/internal/grammar/HCLParserBaseVisitor.java
    • lines 1-0
  • rewrite-hcl/src/main/java/org/openrewrite/hcl/internal/grammar/HCLParserListener.java
    • lines 1-0
  • rewrite-hcl/src/main/java/org/openrewrite/hcl/internal/grammar/HCLParserVisitor.java
    • lines 1-0
  • rewrite-hcl/src/main/java/org/openrewrite/hcl/internal/grammar/JsonPathParserBaseListener.java
    • lines 1-0
  • rewrite-hcl/src/main/java/org/openrewrite/hcl/internal/grammar/JsonPathParserBaseVisitor.java
    • lines 1-0
  • rewrite-hcl/src/main/java/org/openrewrite/hcl/internal/grammar/JsonPathParserListener.java
    • lines 1-0
  • rewrite-hcl/src/main/java/org/openrewrite/hcl/internal/grammar/JsonPathParserVisitor.java
    • lines 1-0

@mccartney
Copy link
Contributor Author

./gradlew licenseFormat

Thanks! Didn't know.
I ran that in fe73bc4 and closed all the bot suggestions.

@mccartney
Copy link
Contributor Author

I need help in deciding what to do with the next batch of bot suggestions here.
Valid or not, but I am not sure if we want to apply bot suggestions against generated code. This seems counterproductive to me, but maybe this is what the project wants to enforce for all code.

Which BTW, to me is a sign this code shouldn't be checked-in to VCS.

@timtebeek
Copy link
Contributor

Thanks a lot for the fixes here @mccartney ; I agree it feels odd to apply recipe fixes to generated code, but that's what we've come to do (for now) to clear out issues on code such that the bot only ever sees net-new issues on PRs. Since the changes are automated I think it's fine for now; longer term we can look at excluding those files from recipe changes.

Copy link
Contributor

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

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

On the subject of the code changes here; would you mind educating me a bit on what you've changed and the process behind regenerating those parsers? 😅 Would help me review & verify, as the changes look good already but I'd like the process to be repeatable for the next one.

@mccartney
Copy link
Contributor Author

mccartney commented Oct 29, 2024

would you mind educating me a bit on what you've changed

Sure:

  • The only file I have edited in this PR myself is the .g4 one. The input to ANTLR.
  • Then I temporarily edited rewrite-hcl/build.gradle file to mention org.antlr:antlr4 instead of org.antlr:antlr4-runtime (turns out to be prerequisite to running generateAntlrSources in current state of things)
  • Then I ran ./gradlew rewrite-hcl:generateAntlrSources, so that ANTLR generates rewrite-hcl/src/main/java/org/openrewrite/hcl/internal/grammar.
  • Then I added a new test of escapingTheDollarSign to ensure the $${something} case is properly handled.

@mccartney
Copy link
Contributor Author

mccartney commented Oct 29, 2024

I'd like the process to be repeatable for the next one.

IMHO the best way for this process be repeatable and self-documented is

Copy link
Contributor

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

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

Great to see these issues resolved so quickly after you've reported them here. Thanks for the helpful pointers as well on a part I hadn't seen much of before. 🙏🏻

@timtebeek timtebeek merged commit 6ba5a6d into openrewrite:main Oct 29, 2024
2 checks passed
@mccartney mccartney deleted the hcl-parser-dollars-4613 branch October 29, 2024 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working parser-hcl
Projects
Archived in project
2 participants