Add a flag to reverse the order of jpath entries. #400
Conversation
| file: String | ||
| ) | ||
| ) { | ||
| def getJpaths: Seq[String] = { |
There was a problem hiding this comment.
It doesn't always reverse the list, it's more of a getter for jpaths which sometimes flips it around based on the flag. Your suggestion sounds to me like we always flip it...
The plain getter-like name isn't optimal either though, so I definitely welcome other suggestions :).
There was a problem hiding this comment.
There are two more SjsonnetMains need this update I think
There was a problem hiding this comment.
Landed on getOrderedJpaths to capture that the order is considered without being overly specific about it.
There are two more SjsonnetMains need this update I think
Can you elaborate please? I fulltext searched and only found the places already edited.
There was a problem hiding this comment.
There are two more in js and scala native folder
There was a problem hiding this comment.
Sorry if I'm being slow, but my understanding of the files is the following:
sjsonnet/src-jvm-native/sjsonnet/SjsonnetMain.scalais the canonical place to handle the imports, which has been updatedsjsonnet/server/src/sjsonnet/SjsonnetServerMain.scalauses the above, and doesn't specify a custom importer, hence the local is used.- The benchmarks use the native SjsonnetMain as well
AFAICT, the Javascript's sjsonnet/src-js/sjsonnet/SjsonnetMain.scala has no local usage, and delegates the import resolving to the caller through importResolver/ImportLoader.
I found no other places where the import paths could be specified. Note that I haven't changed the core, which still evaluates the imports in left-to-right order unconditionally, so the change is only relevant to places which use the Config class parameters.
Of course I may have missed something - please point me to the specific lines if that's the case.
Thanks!
| * implementations. | ||
| */ | ||
| def getOrderedJpaths: Seq[String] = { | ||
| if (jpaths == null) { |
There was a problem hiding this comment.
paths can never be null. Nil is equivalent to an empty list, so I would just drop this condition
| * Historically, sjsonnet evaluated jpaths in left-to-right order, which is also the order of | ||
| * evaluation in the core. However, in gojsonnet, the arguments are prioritized left to right, and | ||
| * the reverse-jpaths-priority flag was introduced for possible consistency across the two | ||
| * implementations. |
There was a problem hiding this comment.
Do you think we should add a link to the original post here?
|
@Danstahrm An update is needed :) |
|
@He-Pin apologies for the delay, I've been a bit too busy lately. I hope to get back to it soon-ish. |
In the canonical implementation of jsonnet, the last match presented on -J wins (see https://jsonnet-libs.github.io/jsonnet-training-course/lesson2.html#jsonnet_path for details). Given the current state is behavior is documented it's undesirable to simply change the implementation as that breaks backwards compatibility so the behavior is hidden behind a feature flag.
In the canonical implementation of jsonnet, the last match presented on -J wins (see
https://jsonnet-libs.github.io/jsonnet-training-course/lesson2.html#jsonnet_path for details). Given the current state is behavior is documented it's undesirable to simply change the implementation as that breaks backwards compatibility so the behavior is hidden behind a feature flag.