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

Extra dollar sign to avoid env var substitution in docs #1241

Merged
merged 2 commits into from
Oct 5, 2023

Conversation

daddykotex
Copy link
Contributor

@daddykotex daddykotex commented Oct 5, 2023

Previous implementation left the dollar sign as is, this triggered a warning:

[warn] [bootstrapped] /Users/David.Francoeur/workspace/dev/smithy4s/modules/bootstrapped/src/generated/smithy4s/example/EnvVarString.scala:12:38: Variable ENV undefined in comment for class EnvVarString in class EnvVarString
[warn] [bootstrapped]   *   This is meant to be used with $ENV_VAR
[warn] [bootstrapped]                                      ^
[warn] [bootstrapped] one warning found

Current implementation uses a double $ sign to avoid this. I tried using {@literal $} but they must be using some sort of basic regex that fails, because when I try, it fails with an Illegal group reference similar to this SO question

@Baccata Baccata merged commit 3442fdf into series/0.18 Oct 5, 2023
10 checks passed
@Baccata Baccata deleted the dfrancoeur/scaladocs branch October 5, 2023 15:01
@Baccata
Copy link
Contributor

Baccata commented Oct 5, 2023

Good thinking !

@daddykotex
Copy link
Contributor Author

I got curious and wondered why literal failed, and looking into the compile I found: https://github.com/scala/scala/blob/40beefbb750d326a5ae71ed852c58dc9a31d7e17/src/scaladoc/scala/tools/nsc/doc/base/CommentFactoryBase.scala#L143

which just wraps the character in backtick. So I gave it a shot and it works:

Screenshot 2023-10-05 at 12 10 23
Screenshot 2023-10-05 at 12 09 02

Do you mind if I open another PR for that?

@daddykotex
Copy link
Contributor Author

Did it anyway: #1242

We can close if not needed

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.

2 participants