Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Potential issue with YAML variables containing dashes #14

Open
erikmd opened this issue Mar 10, 2020 · 7 comments
Open

Potential issue with YAML variables containing dashes #14

erikmd opened this issue Mar 10, 2020 · 7 comments
Labels
question Further information is requested

Comments

@erikmd
Copy link
Member

erikmd commented Mar 10, 2020

Regarding the possible implementations of Mustache, I've successively tested pystache and mo.

On the one hand, pystache seems to be broken:

$ sudo pip3 install pystache

$ pystache meta.yml ../coq-community-templates/.travis.yml.mustache
Traceback (most recent call last):
  File "/usr/local/bin/pystache", line 8, in <module>
    sys.exit(main())
  File "/usr/local/lib/python3.5/dist-packages/pystache/commands/render.py", line 86, in main
    context = json.load(open(context))
  File "/usr/lib/python3.5/json/__init__.py", line 268, in load
    parse_constant=parse_constant, object_pairs_hook=object_pairs_hook, **kw)
  File "/usr/lib/python3.5/json/__init__.py", line 319, in loads
    return _default_decoder.decode(s)
  File "/usr/lib/python3.5/json/decoder.py", line 339, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/usr/lib/python3.5/json/decoder.py", line 357, in raw_decode
    raise JSONDecodeError("Expecting value", s, err.value) from None
json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)
$ 

but on the other hand, mo (the Bash implementation) seems not to recognize YAML variables containing dashes:

$ cd .../coqoban

$ grep mo ../coq-community-templates/generate.sh 
    mo meta.yml "$f" > "$(basename "$f" .mustache)"

$ ../coq-community-templates/generate.sh 
Generating coq.opam...
/usr/local/bin/mo: line 988: opam-file-maintainer: bad substitution
Generating default.nix...
Generating extracted.opam...
Generating index.md...
Generating README.md...
Generating .travis.yml...

hence this issue: should the -'s:

maintainer: "{{& opam-file-maintainer }}{{^ opam-file-maintainer }}[email protected]{{/ opam-file-maintainer }}"

be replaced with _'s like in this other line?

{{# supported_ocaml_versions }}

Finally out of curiosity @palmskog and @Zimmi48, which implementation of mustache are you using?

@erikmd erikmd added bug Something isn't working question Further information is requested labels Mar 10, 2020
@palmskog
Copy link
Member

I'm using the Ruby mustache implementation. Not allowing dashes is quite a big limitation in my book, can you confirm that some other implementation besides the bash one disallows this?

I'm not completely averse to switching to underscores though, since it only seems to be a few fields that use this.

@erikmd
Copy link
Member Author

erikmd commented Mar 10, 2020

Thanks @palmskog, I confirm it was very fine with the Ruby implementation

(for the record, it amounted to installing this APT package: apt-get install ruby-mustache)

Not allowing dashes is quite a big limitation in my book, can you confirm that some other implementation besides the bash one disallows this?

I've only done a few experiments with Bash, Python and Ruby so I can't really confirm this (but BTW it appears Bash and Python do not take a YAML as input: Bash takes environment variables (hence the - limitation in the template syntax it accepts I guess) and Python takes a JSON format).

So maybe this is not really an issue, but indeed:

  • you might want to switch to underscores for uniformity(?)
  • and in any case, it may be useful to recommend the ruby implementation somewhere in the templates repo(?) [all the more as it was the initial implementation]

@erikmd erikmd mentioned this issue Mar 10, 2020
@palmskog
Copy link
Member

palmskog commented Mar 11, 2020

@erikmd I'm open to switching to "underscores only" in all templates, feel free to do a PR with this. You can also add something to the README about the Ruby mustache implementation.

erikmd added a commit to erikmd/templates that referenced this issue Mar 11, 2020
@erikmd
Copy link
Member Author

erikmd commented Mar 11, 2020

@palmskog

You can also add something to the README about the Ruby mustache implementation.

done: #17

I'm open to switching to "underscores only" in all templates, feel free to do a PR with this.

On second thought, I am unsure it is very practical to do such a refactoring, at least now, as the templates repo only contains the .mustache files but not an authoritative sample meta.yml file.

I don't know either if adding a sample meta.yml with underscores in the templates repo would be a solution, as in any case changing the naming convention of the mustache variables would require to change all meta.yml files in the coq-community projects (not only the three mentioned in the templates README) to avoid breaking future updates by these projects' maintainers.

So to sum up: the issue I raised would be only blocking when using these .mustache files with the environment-variables based Bash mustache implementation, but as the overall recommendation is to use .yml metadata files, we could just keep all this as is − and just prefer to use underscore for future new variables, maybe.

(so, feel free to close this issue if need be)

@erikmd erikmd removed the bug Something isn't working label Mar 11, 2020
@palmskog
Copy link
Member

palmskog commented Mar 11, 2020

Not having an authoritative meta.yml was a conscious choice, since I would likely have to update it quite often. See #24 for some ideas on how the metadata format could be specified.

@erikmd
Copy link
Member Author

erikmd commented Mar 11, 2020

OK @palmskog, thanks for the link.

BTW when you say

would likely have to update it quite often.

I agree with you, and I guess furthermore it's an argument for not changing the metadata items names for -/_ in particular, given that this would imply even more updates for existing meta.yml :)

@Zimmi48
Copy link
Member

Zimmi48 commented Mar 16, 2020

FTR I use the Go implementation, which also supports YAML input files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants