Skip to content

Conversation

@stackoverflow
Copy link
Contributor

No description provided.

Copy link
Member

@bioball bioball left a 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)
Copy link
Member

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"?

Suggested change
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.

Copy link
Contributor Author

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) {
Copy link
Member

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.

Suggested change
internal class Builder(sourceText: String, private val version: CompatVersion) {
internal class Builder(sourceText: String, private val grammarVersion: GrammarVersion) {

Copy link
Contributor Author

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'
Copy link
Member

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?

Copy link
Contributor Author

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) +
Copy link
Member

Choose a reason for hiding this comment

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

Two nits:

  1. Can we just make this a number? The "V" seems unnecessary.
  2. 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+

Copy link
Contributor Author

@stackoverflow stackoverflow Oct 24, 2025

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)

@stackoverflow stackoverflow merged commit be0142d into apple:main Oct 28, 2025
4 checks passed
@stackoverflow stackoverflow deleted the formatter-versioning branch October 28, 2025 12:29
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