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

proposal: text/template: support "-" in value maps without "index" #23710

Closed
pbarker opened this issue Feb 6, 2018 · 15 comments
Closed

proposal: text/template: support "-" in value maps without "index" #23710

pbarker opened this issue Feb 6, 2018 · 15 comments

Comments

@pbarker
Copy link
Contributor

@pbarker pbarker commented Feb 6, 2018

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

1.9

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

darwin amd64

What did you do?

tried to use "-" in a template.

https://play.golang.org/p/VQzgZ0v2iNo

What did you expect to see?

I expect to be able to use "-" in keys because that is a widely used separator in keys

What did you see instead?

an error for using that char

@dlsniper
Copy link
Contributor

@dlsniper dlsniper commented Feb 6, 2018

I think that's why index should be used, see: https://play.golang.org/p/Sox6mwzwi44

@pbarker
Copy link
Contributor Author

@pbarker pbarker commented Feb 6, 2018

Yeah the use of index is not ideal. You can see the issues it causes helm here helm/helm#2192 (comment) and the frustration around that. Other templating engines don't suffer from this issue, and I don't believe Go's needs to be restricted in this manner.

@mvdan mvdan changed the title Support "-" in text/template value maps text/template: support "-" in value maps Feb 6, 2018
@mvdan mvdan added the NeedsDecision label Feb 6, 2018
@mvdan mvdan added this to the Go1.11 milestone Feb 6, 2018
@mvdan
Copy link
Member

@mvdan mvdan commented Feb 6, 2018

Adding this feature could be a sliplery slope. For example, if we ever wanted arithmetic expressions in the template language, it would conflict with hyphens in selectors.

I also assume that this proposal is about allowing hyphens/dashes in all names in the template language, not just maps and selectors.

@dlsniper
Copy link
Contributor

@dlsniper dlsniper commented Feb 6, 2018

There is the additional problem that if - support is added, why wouldn't we add support for . , https://play.golang.org/p/e9_NSZ3ob9q ? It may not be used now in Helm, but maybe other apps would want it.

I think that a better rule would be to validate the identifiers and whenever one such identifier is encountered, inform the user that the correct way to represent the fields is using index (which can be done for dots as well, if a bit more work).

@pbarker
Copy link
Contributor Author

@pbarker pbarker commented Feb 6, 2018

Arithmetic expressions could still be supported, it could simply be written that within a value map structure that's not processed.

I'm not looking to add support for . as that's the current separator for the map keys and would cause odd conflicts. However, the use case of - in a key is quite common when asking a user to provide a set of values in yaml or json. The necessary use of index is odd and cumbersome to an end user of the product, if things like that can be eliminated they should be.

@mvdan
Copy link
Member

@mvdan mvdan commented Feb 6, 2018

@grillz what do you mean by "within a value map structure"? The language is parsed statically, so the template parser has no idea if it's going to deal with maps, structs, or whatever other type.

All it knows is what a "selector expression" is, like A.B.C.D. So, how would it know to differentiate A.B - C.D from A.B-C.D?

You can come up with rules here, such as "depends on the spacing", but my point is that it complicates the template language. There must be a very good reason to do that, and I honestly don't see it. It would be much easier for helm to work around the issue, either by discouraging the use of dashes, or by rewriting the template pipelines to use the index function automatically.

@pbarker
Copy link
Contributor Author

@pbarker pbarker commented Feb 6, 2018

don't the templated vals always start with .? Seems fairly simple

@pbarker
Copy link
Contributor Author

@pbarker pbarker commented Feb 6, 2018

a templating language should strive to meet its use cases when it can, unless there is sufficient reason not to. The burden of proof is really on your end as to why this is a bad idea, haven't heard a great answer to that yet.

@theckman
Copy link
Contributor

@theckman theckman commented Feb 6, 2018

@grillz A language, programming or templating, should strive to provide the functionality needed by the users to provide value. The language shouldn't over-complicate itself with sugar or additional features, as that's more code to maintain and limitations to be aware of. This is especially true when there are other means to an end for accomplishing what you want.

As someone requesting a feature change to a language, the burden of proof is on you to explain why the current functionality doesn't meet your needs (from a technical perspective). Why should individuals allocate time to implement and maintain this change? Are the risks worth it, especially knowing that the - character could have other functions (like doing subtraction)? What technical challenges are you hitting with this? Why are the current solutions not ample for your needs?

I think more specifically, why is the index function not good enough for your needs? Why doesn't it work for you?

Answering these questions in a clear way would be helpful in making a determination on the value of adding - as a supported character.

Edit: If the change seems simple enough, how do you feel about making the change to the Go standard library to support it?

@pbarker
Copy link
Contributor Author

@pbarker pbarker commented Feb 6, 2018

I think the technical challenges are pretty well documented with the helm case. The use case of taking yaml or json as an input to a template is widely present in our org, and I imagine that to be the case elsewhere. I think the use case of subtraction is easily handled by that fact that value mapping always begins with ., not to mention using - in keys is likely a far more common use case than subtraction.

As far as the use of index you can see the annoyance felt in the helm issue, this is greatly shared in my org. Most people can't believe its the case and wonder why the limitation. I'm here on behalf of those people, having to use different methods is confusing to an end user. If we mean to introduce confusion to the product, people generally want a reason. I'm in search of that reason.

@cznic
Copy link
Contributor

@cznic cznic commented Feb 7, 2018

This is how PHP was born.

@pbarker
Copy link
Contributor Author

@pbarker pbarker commented Feb 7, 2018

@cznic please argue the merits, pretty sure good languages are built on good logical decisions 🙂

@mvdan
Copy link
Member

@mvdan mvdan commented Feb 10, 2018

don't the templated vals always start with .? Seems fairly simple
[...]
The burden of proof is really on your end as to why this is a bad idea

Sorry, but no - you are proposing a major change to the templating language, so it is the proposal that has to describe with detail its design and why its advantages are greater than its disadvantages.

The proposal should be much more detailed than "dashes should work as map keys" and "seems fairly simple". It may seem simple to you, but I still find this proposed change underdeveloped and bringing more complexity than improvement to the language. You have described Helm's problem, but not so much why this is a sensible change to the language or how it would work.

I'm repurposing this issue as a proposal, as this is clearly not turning out to be a simple topic. I'll leave it up to the proposal review team to have a look, but I presume that they too will want the proposal to be made clearer if it is to move forward.

@mvdan mvdan changed the title text/template: support "-" in value maps proposal: text/template: support "-" in value maps without "index" Feb 10, 2018
@gopherbot gopherbot added the Proposal label Feb 10, 2018
@mvdan mvdan modified the milestones: Go1.11, Proposal Feb 10, 2018
@mvdan mvdan removed the NeedsDecision label Feb 10, 2018
@pbarker
Copy link
Contributor Author

@pbarker pbarker commented Feb 11, 2018

@mvdan thank you for repurposing this into a proposal. As I've stated before, I opened this as an issue because I'm not clear why this wouldn't be the case. I was hoping for an "ah-ha" moment as to the logic behind it, but am still not clear on what that is. I'm also unclear on what complexity this brings to the language. I'll try and restate the benefits:

"-" is a very common separator in keys of both JSON and YAML documents, and JSON and YAML documents are frequently used as inputs to templates. The values in these JSON and YAML documents are commonly inputs from other domains. Rather than need to translate any input that uses the common "-" separator into camelcase, it would be very beneficial to support that as an allowed character.

These are real problems being worked around today causing frustration with the product. From my brief survey of other templating languages, they do not suffer from this limitation. The change of allowing this character from what I can tell would be entirely backwards compatible. Going forward you would simply have a lot more happy customers 🙂

@rsc
Copy link
Contributor

@rsc rsc commented Mar 5, 2018

Sorry, but the solutions are to use index or rename the key. We aren't going to expand the identifier set beyond what's there now. It's really meant for struct fields. For non-alphanumeric (non-Go identifier) keys, please use index.

@rsc rsc closed this Mar 5, 2018
@golang golang locked and limited conversation to collaborators Mar 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.