Feedback based on initial usage

[milesj]

@z-jxy

Initial thoughts/questions after using the library. Here's the example PR attempting to integrate it: moonrepo/schematic#128

[question] Failing test error messages is currently confusing

I've added a few basic tests to try and get it working, which seems to work, but it fails with a weird error message. For the input:

string = "foo"

It fails with:

invalid type: string "foo", expected option

What exactly is an option? How is my string syntax invalid? Is this a Pkl error or your crate?

[question/request] Can non-paths be evaluated?

It looks like evaluate_module requires a path to the .pkl file, but schematic allows for URLs (we download the content) and raw strings as well. Is it possible to support passing the .pkl file contents directly to be evaluated, or is the path a requirement from Pkl itself?

[request] Include span information in errors

I would be nice (if possible) to include span/range/offsets in the parse error, similar to how other serde libraries work. Right now I skip this functionality: https://github.com/moonrepo/schematic/pull/128/files#diff-253e3fd15af0c46c23601f0da24b3543229a85e78521400e997c17faeb8c1378R116

[request] Remove duration_constructors feature

While this feature is now, it forces consumers to not use the stable channel. I don't want to force users of schematic to use nightly.

[z-jxy]

[question] Failing test error messages is currently confusing
What exactly is an option? How is my string syntax invalid? Is this a Pkl error or your crate?

This error comes from the serde deserializer. Based off the Config struct I saw from the PR you mentioned, I'm not seeing why it would be expecting an option when the field is defined as a String. Is that error from trying to deserialize into another struct where it's an Option<String>? I'm able to reproduce the error by making a struct with an optional field. This is probably an implementation I missed in the deserializer. Should be a quick fix once I find the method that needs to be updated.

[question/request] Can non-paths be evaluated?

AFAIK, there isn't a way to evaluate source directly without the pkl interpreter and currently the message passing scheme used by pkl only accepts the path to a file for it to evaluate. I'd like to see support for this too, I'm thinking this should be possible once the C library is released

I would be nice (if possible) to include span/range/offsets in the parse error, similar to how other serde libraries work.

Do you mean if pkl failed to evaluate the module? If so, I think pkl includes this information in the response when evaluating. I could see about returning the error back to the user.

[request] Remove duration_constructors feature

Yeah I agree. I'll see if I can figure out a work around for the duration implementations where it was needed.

Thanks for the feedback!

[milesj]

This error comes from the serde deserializer. Based off the Config struct I saw from the PR you mentioned, I'm not seeing why it would be expecting an option when the field is defined as a String. Is that error from trying to deserialize into another struct where it's an Option<String>? I'm able to reproduce the error by making a struct with an optional field. This is probably an implementation I missed in the deserializer. Should be a quick fix once I find the method that needs to be updated.

Yeah the Config derive generates another struct with fields wrapped in Option, so it's using that to deserialize with.

AFAIK, there isn't a way to evaluate source directly without the pkl interpreter and currently the message passing scheme used by pkl only accepts the path to a file for it to evaluate.

No worries! I can work around it for now.

Do you mean if pkl failed to evaluate the module? If so, I think pkl includes this information in the response when evaluating. I could see about returning the error back to the user.

The span would be the row/col offsets where the parse error was triggered. It's to display these kind of errors in the terminal: https://raw.githubusercontent.com/zkat/miette/main/images/single-line-example.png

Yeah I agree. I'll see if I can figure out a work around for the duration implementations where it was needed.

Much appreciated!

[z-jxy]

Yeah the Config derive generates another struct with fields wrapped in Option, so it's using that to deserialize with.

Got it yeah there was an issue with how the deserializer was structured + the visitor for options wasn't implemented. Just pushed a fix for this

The span would be the row/col offsets where the parse error was triggered.

Problem with this is that Pkl only includes the line where the error occurred in the response

–– Pkl Error ––
Expected value of type `Int`, but got type `String`.
Value: "311"

4 | sup: Int = "311"
         ^^^
at nonprim#sup (file:///path/to/pkl/file.pkl, line 4)

4 | sup: Int = "311"
               ^^^^^
at nonprim#sup (file:///path/to/pkl/file.pkl, line 4)

106 | text = renderer.renderDocument(value)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
at pkl.base#Module.output.text (https://github.com/apple/pkl/blob/0.25.2/stdlib/base.pkl#L106)

It could be possible to calculate the offset for the first error in the response. I'd have to test and see how reliable it ends up being.

[milesj]

It could be possible to calculate the offset for the first error in the response. I'd have to test and see how reliable it ends up being.

If it ends up not being reliable, don't worry too much about it. I can work around it.

[milesj]

@z-jxy I've noticed some commits land on master. Can you release a patch so I can test them? Not everything needs to be done as this is all still experimental.

[z-jxy]

@milesj Yup sorry for delay, release for the latest changes is available as 0.3.1. Should solve the issue with optional struct fields and doesn't require the unstable duration constructors feature.