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

Add update template funcs to change maps and slices #7232

Closed
regisphilibert opened this issue May 1, 2020 · 20 comments
Closed

Add update template funcs to change maps and slices #7232

regisphilibert opened this issue May 1, 2020 · 20 comments

Comments

@regisphilibert
Copy link
Member

regisphilibert commented May 1, 2020

I once wrote a proposal (#5604) for a complex data manipulation API which I now find unnecessary except for the ability to build on maps.

For now the only way to build on maps is either using .Scratch (SetInMap) or the merge function which tends to be much slower.

A problematic data race (#7230 (comment)) was recently surfaced on .Scratch.SetInMap. Applying a patch for it might slow the build of other more common usage.

This proposal explores building on maps without Scratch.

Proposal

Introduce a global function to build on maps

1. Use the existing append function

It is currently reserved for slices. This means the improved append would test for the input data type, and upon finding a map, expect 3 arguments instead of 2:

  • initial map
  • key
  • value

If key already exists, it would be overwritten.

{{ $data := dict }}
{{ $data = append $data "key" "value" }}

2. A new function, yet to be named

It would take 3 parameter

  • initial map
  • key
  • value

If key already exists, it would be overwritten.

{{ $data := dict }}
{{ $data = appendToMap $data "key" "value" }}

3. keyVals

I know we use a keyVals function (undocumented outside of Related Content) I have no idea if this could be useful in this context, but some might chime in.

Also

This would remove the use of .Scratch.SetInMap and allow the future retirement of .Scratch. This was a great feature, but a workaround to now lifted Go Template limitations.

@bep
Copy link
Member

bep commented May 1, 2020

The thing is, from your problematic case earlier:

{{ $data := partialCached "foo" }}
{{ $data = $data  | append "key" "value" }}

The above would have exactly the same concurrency issues; the panic is one thing, but I since multiple threads are working on the same data structure -- the end result seems to be pretty random.

@regisphilibert
Copy link
Member Author

The above would have exactly the same concurrency issues;

Fair enough, but I think this proposal still has value.

@bep
Copy link
Member

bep commented May 1, 2020

I think we need to let append be about slices -- people coming from Go will be confused.

The performance problem with merge is that it makes a copy on every invocation -- which we could avoid by adding a mergeUpdate variant (or something).

Maybe better would be to add a update template func (name?) -- which would in it's name make it clear that we mutate the data you send in.

{{ $data = $data  | update "key" "value" }}

@regisphilibert
Copy link
Member Author

regisphilibert commented May 1, 2020

I think update is great. I've been looking for a term that would make it obvious it's limited to Maps but I can't find anything better so I'm all for update.

Maybe some other languages like Javascript have some wording that make sense?

@bep
Copy link
Member

bep commented May 1, 2020

We should make it work for slices to, so:

{{ $data = $data  | update 0 "value" }}

Would work.

I thought about set as well, but that probably makes people think it creates a Set.

@bep bep added Enhancement and removed Proposal labels May 1, 2020
@bep bep added this to the v0.70 milestone May 1, 2020
@bep bep changed the title Allow append to build on maps Add update template funcs to change maps and slices May 1, 2020
@regisphilibert
Copy link
Member Author

We should make it work for slices to

I'm looking for a use case but can't find one. Beside how could one know which index to update in the slice? Is there a way to identify the index of a value in a slice? An indexOf of some sort.

@bep
Copy link
Member

bep commented May 1, 2020

There is certainly some "shooting in the foot" potential for such a function, (.Params | update "randomparam" "foo"), but we can worry about that later.

An indexOf of some sort.

You can get it when ranging, e.g. {{ range $index, $element := myslice }} -- and I'm pretty sure I got that syntax wrong. Anyway, we can start with maps.

@bep
Copy link
Member

bep commented May 1, 2020

Just got a cool showcase in for the docs site.

@regisphilibert
Copy link
Member Author

regisphilibert commented May 1, 2020

There is certainly some "shooting in the foot" potential for such a function, (.Params | update "randomparam" "foo"), but we can worry about that later.

Same for .Params.authors | append "New Author" I guess...

Are .Params and dict "key" "value" of the same data type, can't we forbid some data types as input?

@bep
Copy link
Member

bep commented May 1, 2020

Same for .Params.authors | append "New Author" I guess...

No, that does not change the slice.

@moorereason
Copy link
Contributor

update seems too vague to me. I like set, but had the same reaction as @bep -- it's also a little too vague.

What about setitem?

{{ $data = $data  | setitem "key" "value" }}

@bep
Copy link
Member

bep commented May 1, 2020

{{ $data = $data  | put "key" "value" }}

???

@regisphilibert
Copy link
Member Author

Maybe ask the discourse for ideas?

@regisphilibert
Copy link
Member Author

regisphilibert commented May 2, 2020

put, update, or assign which could be another lead, tends to let the user think more than one can be set.

I think set is the way to go, it is short, and aalready used in Javascript (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map/set)

If we want to make it less evasive, maybe we could start wrapping data modifying functions in a keywords matching the type: map.Merge, map.Set, map.Create (keeping dict as an alias) slice.Append etc...

@moorereason
Copy link
Contributor

@regisphilibert, your set example from Javascript is a method on Map objects, not a function, so there's inherently less ambiguity.

Of the options presented so far, I still like setitem the most. I found a couple other implementations using this naming convention:

  • Python uses __setitem__ and __getitem__ to provide map-like functionality to classes.
  • The Web Storage API uses setItem with a key+value pair. Granted these are methods, but it just shows that there are other approaches in the Javascript community.

While googling around, I saw another option: the Rust std::collections::HashMap library uses insert to add/update a map. Again, it's a method. Not sure what I think about this option yet.

@regisphilibert
Copy link
Member Author

I like insert.
setitem is also good, it's just hard to read without case, I'm always struggling with getenv :)

@instance-id
Copy link

instance-id commented May 7, 2020

Is there no way at all to do this currently?

I have a toml data template/file that I am having "Jsonified" based on a json.json file and I was leaving the date field set as "" expecting to be able to do something like:

(Shortened)

{{ range $log := $chlog }}
{{ if eq (index $log "date") "" }}
{{  (index $log "date") = (now.Format "2006-01-01") }}

Which didn't work, so I tried to see if this would:
(shortened)

{{.Scratch.Add "tomlData" $chlog}}
{{ range (.Scratch.Get "tomlData") }}
{{ if (eq (index . "date") "") }}
{{ .Scratch.SetInMap "tomlData" (index . "date") (now.Format "2006-01-01") }}
{{end}}
{{end}}
{{- jsonify (.Scratch.Get "tomlData") -}}

That didn't work either. Is there some way to replace a blank value in a map

@moorereason
Copy link
Contributor

@instance-id, please use the discussion forums for help with using Hugo.

@stale
Copy link

stale bot commented Aug 16, 2020

This issue has been automatically marked as stale because it has not had recent activity. The resources of the Hugo team are limited, and so we are asking for your help.
If this is a bug and you can still reproduce this error on the master branch, please reply with all of the information you have about it in order to keep the issue open.
If this is a feature request, and you feel that it is still relevant and valuable, please tell us why.
This issue will automatically be closed in the near future if no further activity occurs. Thank you for all your contributions.

@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants