-
Notifications
You must be signed in to change notification settings - Fork 338
Add grammar compatibility option to the formatter #1249
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving to unblock, but, we should figure out what to name this.
Also: we should update the docs.
| .multiple() | ||
|
|
||
| val compatVersion: CompatVersion by | ||
| option(names = arrayOf("-v", "--version"), help = compatVersionHelp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't conflict with the built-in --version flag that already exists.
Names are hard, but, maybe let's call this "grammar version"?
| option(names = arrayOf("-v", "--version"), help = compatVersionHelp) | |
| option(names = arrayOf("--grammar-version"), help = compatVersionHelp) |
I also don't think we need to offer a short version of this flag; descriptive flags help with readability of scripts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed everything to be grammar version and removed the short version. The other good name would be grammar compatibility version, but that's too verbose.
| import org.pkl.parser.syntax.generic.NodeType | ||
|
|
||
| internal class Builder(sourceText: String) { | ||
| internal class Builder(sourceText: String, private val version: CompatVersion) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whatever name we settle on, we should change this and the class name too.
| internal class Builder(sourceText: String, private val version: CompatVersion) { | |
| internal class Builder(sourceText: String, private val grammarVersion: GrammarVersion) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
| import org.pkl.commons.cli.commands.single | ||
|
|
||
| private const val NEWLINE = '\u0085' | ||
| internal const val NEWLINE = '\u0085' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move to a shared place, maybe a util.kt or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to a utils file.
| .--grammar-version | ||
| [%collapsible] | ||
| ==== | ||
| Default: `V2` (latest version) + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two nits:
- Can we just make this a number? The "V" seems unnecessary.
- Can we provide a table of grammar version to language version? Something like:
| Grammar Version | Pkl Version |
|---|---|
| 1 | 0.25 - 0.29 |
| 2 | 0.30+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cli option just requires a number now. I actually just used a number in the beginning but changed it later.
The description for each version is the version span it supports.
--grammar-version=(1|2) The grammar compatibility version to use.
1: 0.25 - 0.29
2: 0.30+ (default)02d8600 to
329ea79
Compare
No description provided.