Skip to content

Commit fac5260

Browse files
HT154HT154
committed
Address review feedback
1 parent f0555fc commit fac5260

File tree

7 files changed

+63
-122
lines changed

7 files changed

+63
-122
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/ast/member/ClassProperty.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@
1616
package org.pkl.core.ast.member;
1717

1818
import com.oracle.truffle.api.source.SourceSection;
19-
import java.util.ArrayList;
2019
import java.util.List;
2120
import org.pkl.core.Member.SourceLocation;
2221
import org.pkl.core.PClass;

‎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: 55 additions & 98 deletions
Original file line numberDiff line numberDiff line change
@@ -228,48 +228,30 @@ 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]).isInstanceOf(CommandSpec.Flag::class.java)
234+
(spec.options.toList()[0] as CommandSpec.Flag).apply {
235+
assertThat(this.name).isEqualTo("bar")
236+
assertThat(this.shortName).isEqualTo("x")
237+
assertThat(this.helpText).isEqualTo("bar in Options")
238+
}
239+
240+
// assert first flag annotation wins
241+
assertThat(spec.options.toList()[1]).isInstanceOf(CommandSpec.Flag::class.java)
242+
(spec.options.toList()[1] as CommandSpec.Flag).apply {
243+
assertThat(this.name).isEqualTo("baz")
244+
assertThat(this.shortName).isEqualTo("y")
245+
assertThat(this.helpText).isEqualTo("baz in Options")
246+
}
247+
248+
// assert superclass options are inherited
249+
assertThat(spec.options.toList()[2]).isInstanceOf(CommandSpec.Flag::class.java)
250+
(spec.options.toList()[2] as CommandSpec.Flag).apply {
251+
assertThat(this.name).isEqualTo("foo")
252+
assertThat(this.shortName).isEqualTo("a")
253+
assertThat(this.helpText).isEqualTo("foo in BaseOptions")
254+
}
273255
}
274256

275257
@Test
@@ -406,68 +388,43 @@ class CommandSpecParserTest {
406388
bar: String = foo
407389
baz: Map<String, String> = Map()
408390
qux: Map<String, String> = baz
391+
quux: Int = 5
409392
}
410393
"""
411394
.trimIndent(),
412395
)
413396

414397
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-
// )
398+
399+
assertThat(spec.options.toList()[0]).isInstanceOf(CommandSpec.Flag::class.java)
400+
(spec.options.toList()[0] as CommandSpec.Flag).apply {
401+
assertThat(this.name).isEqualTo("foo")
402+
assertThat(this.defaultValue).isEqualTo("hi")
403+
}
404+
405+
assertThat(spec.options.toList()[1]).isInstanceOf(CommandSpec.Flag::class.java)
406+
(spec.options.toList()[1] as CommandSpec.Flag).apply {
407+
assertThat(this.name).isEqualTo("bar")
408+
assertThat(this.defaultValue).isNull()
409+
}
410+
411+
assertThat(spec.options.toList()[2]).isInstanceOf(CommandSpec.Flag::class.java)
412+
(spec.options.toList()[2] as CommandSpec.Flag).apply {
413+
assertThat(this.name).isEqualTo("baz")
414+
assertThat(this.defaultValue).isNull()
415+
}
416+
417+
assertThat(spec.options.toList()[3]).isInstanceOf(CommandSpec.Flag::class.java)
418+
(spec.options.toList()[3] as CommandSpec.Flag).apply {
419+
assertThat(this.name).isEqualTo("qux")
420+
assertThat(this.defaultValue).isNull()
421+
}
422+
423+
assertThat(spec.options.toList()[4]).isInstanceOf(CommandSpec.Flag::class.java)
424+
(spec.options.toList()[4] as CommandSpec.Flag).apply {
425+
assertThat(this.name).isEqualTo("quux")
426+
assertThat(this.defaultValue).isEqualTo("5")
427+
}
471428
}
472429

473430
@Test

0 commit comments

Comments
 (0)