Error when JSON/YAML parsing would set property default in a Dynamic
#700
Conversation
HT154
force-pushed
the
parse-dynamic-disallow-default
branch
from
8a2b659 to
109101e
Compare
There was a problem hiding this comment.
Thanks for the PR! Just some comments about error messages.
CC @holzensp @stackoverflow; merging this is a breaking change.
pkl-core/src/main/java/org/pkl/core/stdlib/json/ParserNodes.java
Outdated
Show resolved
Hide resolved
pkl-core/src/main/java/org/pkl/core/stdlib/yaml/ParserNodes.java
Outdated
Show resolved
Hide resolved
pkl-core/src/main/java/org/pkl/core/stdlib/json/ParserNodes.java
Outdated
Show resolved
Hide resolved
pkl-core/src/main/java/org/pkl/core/stdlib/yaml/ParserNodes.java
Outdated
Show resolved
Hide resolved
| @@ -469,6 +473,13 @@ private void addMembers(MappingNode node, VmObject object) { | |||
| var memberName = | |||
| convertedKey instanceof String string && !useMapping ? Identifier.get(string) : null; | |||
|
|
|||
| // https://github.com/apple/pkl/issues/561 | |||
| if (memberName != null && "default".equals(convertedKey)) { | |||
| throw new ParseException( | |||
There was a problem hiding this comment.
All other error handling code in this file throws YamlEngineException, which is caught in the same file and translated to VmException. Why throw a custom exception here, only to duplicate the code that translates to VmException? (I know this was suggested in the review, but I don't see the point.)
There was a problem hiding this comment.
YamlEngineException doesn't feel like the right class to throw our own errors with tailored messages. I think the direction we should move here is to switch all of our own throws from YamlEngineException to org.pkl.core.util.yaml.ParseException.
There was a problem hiding this comment.
Throwing ParseException everywhere is an improvement. But since these exceptions are implementation details and thrown and caught in the same file, introducing your own exception feels unnecessary (adds code for no benefit).
json.ParserNodes follows the same pattern: It throws org.pkl.core.util.json.ParseException, which also isn't your own exception.
There was a problem hiding this comment.
I agree that the extra class seems needlessly verbose and that we should probably move away from YamlEngineExceptions. I don't quite understand org.pkl.core.util.json.ParseException not being "your own exception." What do you mean by that, @translatenix?
Is there any reason not to move org.pkl.core.util.json.ParseException up one level (into util) and use it for either?
There was a problem hiding this comment.
The idea is that errors coming from ParseException can be presented to users as-is; because they are already tailored to be presented as error messages.
YamlEngineException messages come from SnakeYAML, and should be decorated with a generic "malformed yaml" message.
I think a good north star here is to not surface messages from YamlEngineException at all, and instead present all parse errors as our own tailored error messages. But, for now, it's helpful to have two different exception classes so we know what comes from us, and what comes from our parser library.
There was a problem hiding this comment.
I don't quite understand org.pkl.core.util.json.ParseException not being "your own exception." What do you mean by that, @translatenix?
org.pkl.core.util.json.ParseException comes from the external JSON library that was copied into Pkl's codebase. As such it's the equivalent of YamlEngineException .
YamlEngineException messages come from SnakeYAML, and should be decorated with a generic "malformed yaml" message.
If that's what you want, having your own exception makes sense, assuming you can't throw a VmException to begin with. You may want to make the same change for json/ParserNodes.
HT154
force-pushed
the
parse-dynamic-disallow-default
branch
2 times, most recently
from
e5504b5 to
ed92c3e
Compare
pkl-core/src/main/resources/org/pkl/core/errorMessages.properties
Outdated
Show resolved
Hide resolved
pkl-core/src/main/resources/org/pkl/core/errorMessages.properties
Outdated
Show resolved
Hide resolved
pkl-core/src/main/resources/org/pkl/core/errorMessages.properties
Outdated
Show resolved
Hide resolved
| @@ -469,6 +473,13 @@ private void addMembers(MappingNode node, VmObject object) { | |||
| var memberName = | |||
| convertedKey instanceof String string && !useMapping ? Identifier.get(string) : null; | |||
|
|
|||
| // https://github.com/apple/pkl/issues/561 | |||
| if (memberName != null && "default".equals(convertedKey)) { | |||
| throw new ParseException( | |||
There was a problem hiding this comment.
I agree that the extra class seems needlessly verbose and that we should probably move away from YamlEngineExceptions. I don't quite understand org.pkl.core.util.json.ParseException not being "your own exception." What do you mean by that, @translatenix?
Is there any reason not to move org.pkl.core.util.json.ParseException up one level (into util) and use it for either?
HT154
force-pushed
the
parse-dynamic-disallow-default
branch
3 times, most recently
from
f5f96df to
13e3306
Compare
Co-authored-by: Philip K.F. Hölzenspies <[email protected]>
HT154
force-pushed
the
parse-dynamic-disallow-default
branch
from
13e3306 to
9ee33e3
Compare
Resolves #561