direct yaml importing breaks importstr

[sh0rez]

In #106 we introduced direct importing of yaml documents (as they are similar enough to json) by wrapping the jsonnet.FileImporter and extending it by detecting all .yaml documents. In case it is indeed yaml, we return the contents as json, instead of the literal contents.

This however is problematic in combination with importstr, which should result in literal imports. But as the same importer is used, we also get the JSON contents here. Even worse, the jsonnet.Importer interface does not allow us to detect which import mode we operate on.

Any ideas how that could be solved @sbarzowski?

[issue-label-bot]

Issue-Label Bot is automatically applying the label kind/bug to this issue, with a confidence of 0.81. Please mark this comment with 👍 or 👎 to give our bot feedback!

Links: app homepage, dashboard and code for this bot.

[sbarzowski]

Even worse, the jsonnet.Importer interface does not allow us to detect which import mode we operate on.

This is very much by design. Importer is supposed to provide a "data layer". It should be possible to move the data to another storage system and use a different importer easily.

Any ideas how that could be solved @sbarzowski?

Considering that people seem quite enthusiastic about this feature here, we can consider adding an upstream support for YAML/JSON parsing based on the imported path. It's common to see import "sth.json" instead of more proper std.parseJson(importstr "str.json"), so it makes sense to just do the right thing automatically.

The parsing would need to happen outside of the importer. We would need to detect the filetype based on any path supported by the importer (for example it should work with the following notation for some imaginary importer: my.jsonnet.package.file:yaml). So probably a new method for the Importer interface along the lines of GetFileType(path) -> enum(Jsonnet, JSON, YAML, Nothing) would be needed.

Another approach (slightly incompatible for Tanka users) would be to introduce new keywords importjson, importyaml.

Of course upstream support will require a discussion and if we decide to do it, it may still take a while. Other incompatible changes are coming to the Importer (due to #329) so it may be a good time to consider this.

@sparkprime, @lihaoyi What do you think about path-based detection of JSON and YAML?

Short of adding this sort of upstream support, I don't see a very good way out. The way we recommended so far upstream is std.parseYAML(importstr "sth.yaml"), but I see how going back to it would suck for your users. One way out of it, which can be implemented within the importer, is to introduce a "virtual" path. That is, if the user imports something like "blah/blah/file.yaml#raw" you can resolve it like "blah/blah/file.yaml", but without the magic conversion. This is pretty hacky of course.

[sh0rez]

Upstream support is the most proper solution and I am happy to wait for it. Until then we can just document this issue and are good to go.

Regarding the syntax, I feel like a generic import is the nicest approach. Using import for all JSON-like things (Jsonnet, JSON, YAML, maybe TOML) and importstr for everything else.

This is also non-breaking to the current workflow. I am happy to help implementing this!

[lihaoyi]

I see a lot of `import foo.json` in our codebase. It might be a bit finnicky to have it guess based on file extensions, but probably not a huge deal. In the end Sjsonnet’s importer is swappable, so if we did this the special-file-extension handler would just become swappable as well

…

On Tue, 7 Jan 2020 at 2:18 AM, sh0rez ***@***.***> wrote: Upstream support is the most proper solution and I am happy to wait for it. Until then we can just document this issue and are good to go. Regarding the syntax, I feel like a generic import is the nicest approach. Using import for all JSON-like things (Jsonnet, JSON, YAML, maybe TOML) and importstr for everything else. This is also non-breaking to the current workflow. I am happy to help implementing this! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#135?email_source=notifications&email_token=AAHEB7HX5YJY6MRCSA4RXCTQ4NYWPA5CNFSM4KDGU72KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIGJH7Q#issuecomment-571249662>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHEB7EOALEANMWNLDT63G3Q4NYWPANCNFSM4KDGU72A> .

[DanCech]

This issue causes a major BC break for any projects using importstr 'some.yaml' as it now results in the yaml being parsed as if it were imported via import.

Is there a reason that any file imported via importstr is treated as anything other than an opaque string?

[sh0rez]

No, it's a bug. Because @tomwilkie wished for it, we allowed to import yaml files, but had to implement this in the importer (it intercepts the import and instead of returning the contents, returns the object).

The importer however is used on import and importstr (which I was not aware of), so I accidentally broke this. To really fix this, we need to wait for changes to google/go-jsonnet

[DanCech]

Since this will break anyone using importstr with yaml files in a pretty nasty way, should we consider backing out #106 until it can be implemented in a way that doesn't have that side effect?

[sh0rez]

For now, this is fixed in #176. Tracking for reimplementing does #177

[DanCech]

Thanks @sh0rez ! any chance of cutting v0.6.2?