Skip to content

Commit 1b266d5

Browse files
HT154HT154
committed
Address review feedback
1 parent d37634f commit 1b266d5

File tree

6 files changed

+71
-121
lines changed

6 files changed

+71
-121
lines changed

‎pkl-cli/src/main/kotlin/org/pkl/cli/CliCommandRunner.kt‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -85,11 +85,7 @@ constructor(
8585
}
8686
}
8787

88-
/**
89-
* Renders the comand's `output.bytes`, writing it to the standard output stream.
90-
*
91-
* Unlike CliEvaluator, there is no need to handle
92-
*/
88+
/** Renders the comand's `output.bytes`, writing it to the standard output stream. */
9389
fun writeOutput(outputBytes: ByteArray) {
9490
if (outputBytes.isEmpty()) return
9591
outputStream.write(outputBytes)

‎pkl-core/src/main/java/org/pkl/core/PClassInfo.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright © 2024-2026 Apple Inc. and the Pkl project authors. All rights reserved.
2+
* Copyright © 2024-2025 Apple Inc. and the Pkl project authors. All rights reserved.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.

‎pkl-core/src/main/java/org/pkl/core/runtime/CommandSpecParser.java‎

Lines changed: 3 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -310,21 +310,15 @@ private CommandSpec.CountedFlag collectCountedFlag(ClassProperty prop, VmTyped f
310310

311311
// assert type is integral
312312
var typeInfo = resolveType(prop);
313-
if (!(typeInfo.getFirst() instanceof TypeNode.IntTypeNode
314-
|| typeInfo.getFirst() instanceof TypeNode.Int8TypeAliasTypeNode
315-
|| typeInfo.getFirst() instanceof TypeNode.Int16TypeAliasTypeNode
316-
|| typeInfo.getFirst() instanceof TypeNode.Int32TypeAliasTypeNode
317-
|| typeInfo.getFirst() instanceof TypeNode.UIntTypeAliasTypeNode // also UInt16, UInt32
318-
|| typeInfo.getFirst() instanceof TypeNode.UInt8TypeAliasTypeNode)
319-
|| typeInfo.getSecond()) {
313+
if (typeInfo.getFirst().getVmClass() != BaseModule.getIntClass() || typeInfo.getSecond()) {
320314
throw exceptionBuilder()
321315
.withSourceSection(prop.getHeaderSection())
322316
.evalError(
323317
"commandFlagInvalidType",
324318
prop.getName(),
325319
"CountedFlag",
326320
typeInfo.getFirst().getSourceSection().getCharacters(),
327-
"Int | Int8 | Int16 | Int32 | UInt | UInt8 | UInt16 | UInt32")
321+
"Int")
328322
.build();
329323
}
330324

@@ -389,6 +383,7 @@ private Pair<TypeNode, Boolean> resolveType(TypeNode typeNode) {
389383
} else if (typeNode instanceof TypeNode.TypeAliasTypeNode typeAliasTypeNode) {
390384
if (typeAliasTypeNode.getVmTypeAlias() == BaseModule.getCharTypeAlias()) break;
391385
typeNode = typeAliasTypeNode.getAliasedTypeNode();
386+
break;
392387
} else {
393388
break;
394389
}
@@ -1139,7 +1134,6 @@ private Object handleImport(VmTyped mport, URI workingDirUri) {
11391134
.withHint(e.getReason())
11401135
.build();
11411136
}
1142-
;
11431137

11441138
var isGlob = (Boolean) VmUtils.readMember(mport, Identifier.GLOB);
11451139
var importUri = URI.create(uriString);
@@ -1191,11 +1185,6 @@ private Object handleImport(VmTyped mport, URI workingDirUri) {
11911185
return result instanceof PNull ? null : (String) result;
11921186
}
11931187

1194-
private static @Nullable Boolean exportNullableBoolean(VmObjectLike value, Object key) {
1195-
var result = VmValue.export(VmUtils.readMember(value, key));
1196-
return result instanceof PNull ? null : (Boolean) result;
1197-
}
1198-
11991188
/** Check that a value is a VmTyped and that it inherits from the given class */
12001189
private VmTyped checkAmends(Object value, VmClass clazz) {
12011190
if (!(value instanceof VmTyped typed)) {

‎pkl-core/src/main/java/org/pkl/core/util/GlobResolver.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -489,8 +489,8 @@ public static Map<String, ResolvedGlobElement> resolveGlob(
489489
var result = new LinkedHashMap<String, ResolvedGlobElement>();
490490
var hasAbsoluteGlob = globPattern.matches("\\w+:.*");
491491
if ((enclosingModuleKey == null || enclosingUri == null) && !hasAbsoluteGlob) {
492-
// callers must check that the glob is absolute if calling with null enclosing info
493-
throw PklBugException.unreachableCode();
492+
throw new PklBugException(
493+
"GlobResolver.resolveGlob() callers must check that the glob pattern is absolute if calling with null enclosing module info");
494494
}
495495

496496
if (reader.hasHierarchicalUris()) {

‎pkl-core/src/main/resources/org/pkl/core/errorMessages.properties‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1090,7 +1090,7 @@ commandOptionsTypeNotClass=\
10901090
Type annotation `{0}` on `options` property in `pkl:Command` subclass must be a class type.
10911091

10921092
commandOptionsTypeAbstractClass=\
1093-
Class `{0}` for type annotation on `options` property in `pkl:Command` subclass must not be abstract.
1093+
Command options class `{0}` may not be abstract.
10941094

10951095
commandOptionBothFlagAndArgument=\
10961096
Found both `@Flag` and `@Argument` annotations for options property `{0}`.\n\

‎pkl-core/src/test/kotlin/org/pkl/core/runtime/CommandSpecParserTest.kt‎

Lines changed: 63 additions & 98 deletions
Original file line numberDiff line numberDiff line change
@@ -228,48 +228,33 @@ class CommandSpecParserTest {
228228
)
229229

230230
val spec = parse(moduleUri)
231-
// assertThat(spec.options.toList()[0])
232-
// .usingRecursiveComparison()
233-
// .isEqualTo(
234-
// CommandSpec.Flag(
235-
// "bar",
236-
// "x",
237-
// CommandSpec.OptionType.Primitive(CommandSpec.OptionType.Primitive.Type.STRING,
238-
// true),
239-
// null,
240-
// null,
241-
// "bar in Options",
242-
// false,
243-
// )
244-
// )
245-
// assertThat(spec.options.toList()[1])
246-
// .usingRecursiveComparison()
247-
// .isEqualTo(
248-
// CommandSpec.Flag(
249-
// "baz",
250-
// "y",
251-
// CommandSpec.OptionType.Primitive(CommandSpec.OptionType.Primitive.Type.STRING,
252-
// true),
253-
// null,
254-
// null,
255-
// "baz in Options",
256-
// false,
257-
// )
258-
// )
259-
// assertThat(spec.options.toList()[2])
260-
// .usingRecursiveComparison()
261-
// .isEqualTo(
262-
// CommandSpec.Flag(
263-
// "foo",
264-
// "a",
265-
// CommandSpec.OptionType.Primitive(CommandSpec.OptionType.Primitive.Type.STRING,
266-
// true),
267-
// null,
268-
// null,
269-
// "foo in BaseOptions",
270-
// false,
271-
// )
272-
// )
231+
232+
// assert class overrides its superclass
233+
assertThat(spec.options.toList()[0])
234+
.isInstanceOf(CommandSpec.Flag::class.java)
235+
(spec.options.toList()[0] as CommandSpec.Flag).apply {
236+
assertThat(this.name).isEqualTo("bar")
237+
assertThat(this.shortName).isEqualTo("x")
238+
assertThat(this.helpText).isEqualTo("bar in Options")
239+
}
240+
241+
// assert first flag annotation wins
242+
assertThat(spec.options.toList()[1])
243+
.isInstanceOf(CommandSpec.Flag::class.java)
244+
(spec.options.toList()[1] as CommandSpec.Flag).apply {
245+
assertThat(this.name).isEqualTo("baz")
246+
assertThat(this.shortName).isEqualTo("y")
247+
assertThat(this.helpText).isEqualTo("baz in Options")
248+
}
249+
250+
// assert superclass options are inherited
251+
assertThat(spec.options.toList()[2])
252+
.isInstanceOf(CommandSpec.Flag::class.java)
253+
(spec.options.toList()[2] as CommandSpec.Flag).apply {
254+
assertThat(this.name).isEqualTo("foo")
255+
assertThat(this.shortName).isEqualTo("a")
256+
assertThat(this.helpText).isEqualTo("foo in BaseOptions")
257+
}
273258
}
274259

275260
@Test
@@ -406,68 +391,48 @@ class CommandSpecParserTest {
406391
bar: String = foo
407392
baz: Map<String, String> = Map()
408393
qux: Map<String, String> = baz
394+
quux: Int = 5
409395
}
410396
"""
411397
.trimIndent(),
412398
)
413399

414400
val spec = parse(moduleUri)
415-
// val str = CommandSpec.OptionType.Primitive(CommandSpec.OptionType.Primitive.Type.STRING,
416-
// true)
417-
// assertThat(spec.flags[0])
418-
// .usingRecursiveComparison()
419-
// .isEqualTo(
420-
// CommandSpec.Flag(
421-
// "foo",
422-
// null,
423-
// CommandSpec.OptionType.Primitive(CommandSpec.OptionType.Primitive.Type.STRING,
424-
// false),
425-
// "hi",
426-
// null,
427-
// null,
428-
// false,
429-
// )
430-
// )
431-
// assertThat(spec.flags[1])
432-
// .usingRecursiveComparison()
433-
// .isEqualTo(
434-
// CommandSpec.Flag(
435-
// "bar",
436-
// null,
437-
// CommandSpec.OptionType.Primitive(CommandSpec.OptionType.Primitive.Type.STRING,
438-
// false),
439-
// null,
440-
// null,
441-
// null,
442-
// false,
443-
// )
444-
// )
445-
// assertThat(spec.flags[2])
446-
// .usingRecursiveComparison()
447-
// .isEqualTo(
448-
// CommandSpec.Flag(
449-
// "baz",
450-
// null,
451-
// CommandSpec.OptionType.Map(str, str, false),
452-
// null,
453-
// null,
454-
// null,
455-
// false,
456-
// )
457-
// )
458-
// assertThat(spec.flags[3])
459-
// .usingRecursiveComparison()
460-
// .isEqualTo(
461-
// CommandSpec.Flag(
462-
// "qux",
463-
// null,
464-
// CommandSpec.OptionType.Map(str, str, false),
465-
// null,
466-
// null,
467-
// null,
468-
// false,
469-
// )
470-
// )
401+
402+
assertThat(spec.options.toList()[0])
403+
.isInstanceOf(CommandSpec.Flag::class.java)
404+
(spec.options.toList()[0] as CommandSpec.Flag).apply {
405+
assertThat(this.name).isEqualTo("foo")
406+
assertThat(this.defaultValue).isEqualTo("hi")
407+
}
408+
409+
assertThat(spec.options.toList()[1])
410+
.isInstanceOf(CommandSpec.Flag::class.java)
411+
(spec.options.toList()[1] as CommandSpec.Flag).apply {
412+
assertThat(this.name).isEqualTo("bar")
413+
assertThat(this.defaultValue).isNull()
414+
}
415+
416+
assertThat(spec.options.toList()[2])
417+
.isInstanceOf(CommandSpec.Flag::class.java)
418+
(spec.options.toList()[2] as CommandSpec.Flag).apply {
419+
assertThat(this.name).isEqualTo("baz")
420+
assertThat(this.defaultValue).isNull()
421+
}
422+
423+
assertThat(spec.options.toList()[3])
424+
.isInstanceOf(CommandSpec.Flag::class.java)
425+
(spec.options.toList()[3] as CommandSpec.Flag).apply {
426+
assertThat(this.name).isEqualTo("qux")
427+
assertThat(this.defaultValue).isNull()
428+
}
429+
430+
assertThat(spec.options.toList()[4])
431+
.isInstanceOf(CommandSpec.Flag::class.java)
432+
(spec.options.toList()[4] as CommandSpec.Flag).apply {
433+
assertThat(this.name).isEqualTo("quux")
434+
assertThat(this.defaultValue).isEqualTo("5")
435+
}
471436
}
472437

473438
@Test

0 commit comments

Comments
 (0)