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

tpl/math: Allow variadic math functions to take slice args #11030

Closed
jmooring opened this issue May 27, 2023 · 9 comments · Fixed by #11091
Closed

tpl/math: Allow variadic math functions to take slice args #11030

jmooring opened this issue May 27, 2023 · 9 comments · Fixed by #11091
Assignees
Milestone

Comments

@jmooring
Copy link
Member

jmooring commented May 27, 2023

#10638 changed add, sub, mul, div, main, and max into variadic functions. So you can do this:

{{ math.Min 6 7 42 }}
{{ math.Max 6 7 42 }}
{{ math.Add 6 7 42 }}
{{ math.Mul 6 7 42 }}

I'm finding this would be much more useful:

{{ math.Min (slice 6 7 42) }}
{{ math.Max (slice 6 7 42) }}
{{ math.Add (slice 6 7 42) }}
{{ math.Mul (slice 6 7 42) }}

And these should be valid as well:

{{ math.Min 6 7 }}
{{ math.Min (slice 6) 7 }}
{{ math.Min (slice 6) (slice 7) }}
{{ math.Min (slice 6) (slice 7) 42 }}

I suppose we should do the same with math.Sub and math.Div.

The recently implemented urls.PathJoin follows this model.

They should still throw an error with a single arg. For example

{{ math.Sub 6 }} --> error
{{ math.Sub (slice 6) }} --> error
@jmooring
Copy link
Member Author

jmooring commented May 27, 2023

In retrospect, we should have:

  1. Not changed math.Add, math.Sub, math.Mul, and math.Div (require 2 scalar values)
  2. Changed math.Minand math.Max (accept 1 or more values, scalar or slice, or combination of the two)
  3. Introduced math.Sum (1 or more values, scalar or slice, or combination of the two)
  4. Introduced math.Product (1 or more values, scalar or slice, or combination of the two)

Excel has sum and product functions, but does not have variadic functions for subtraction or division. I suspect that's because they aren't terribly useful.

Items 2-4 would be backwards compatible. Thoughts?

@bep bep added this to the v0.113.0 milestone May 27, 2023
@bep bep added Proposal and removed Enhancement labels May 27, 2023
@bep
Copy link
Member

bep commented May 27, 2023

I'm a little slow tonight, but why couldn't we make

{{ math.Add 6 7 42 }}
{{ math.Add (slice 6 7 42) }}

Both work?

@jmooring
Copy link
Member Author

My thinking was:

math.Add should error if < 2 args (e.g., math.Add 6 doesn't make sense -- this is the current behavior)
math.Sub should error if < 2 args (e.g., math.Sub 6 doesn't make sense -- this is the current behavior)
math.Mul should error if < 2 args (e.g., math.Mul 6 doesn't make sense -- this is the current behavior)
math.Div should error if < 2 args (e.g., math.Div 6 doesn't make sense -- this is the current behavior)

math.Sum should error if < 1 arg
math.Product should error if < 1 arg
math.Min should error if < 1 arg
math.Max should error if < 1 arg

When building a slice for math.Sum, math.Product, math.Min, or math.Max through iteration, it is possible to have just one iteration (e.g., ranging through a section with only one page).

@bep
Copy link
Member

bep commented May 28, 2023

math.Add should error if < 2 args (e.g., math.Add 6 doesn't make sense -- this is the current behavior)

Sure, but it shouldn't be too hard to expand on that logic and define args as either number of args or, if number of arg is 1 and the arg is a slice, number of values in the slice.

So I agree that we should do what this issue describes, but we cannot (and I don't see why we would have to) remove the option to say e.g. {{ math.Add 1 2 }}.

@bep bep added Enhancement and removed Proposal labels May 28, 2023
@jmooring
Copy link
Member Author

If I understand correctly, you are suggesting:

{{ math.Add 6 }} --> error (this is the current behavior, needs > 1 arg)

{{ math.Add (slice 6) }} --> 6

I find that confusing for a couple of reasons:

  1. To me, math.Add is (conceptually) the + operator, and neither 6 + nor [6] + seem right
  2. The documentation would be something like, "Requires 2 or more arguments, or 1 argument if it is a slice of one or elements"

My preference is to:

  1. Leave math.Add, math.Sub, math.Mul, and math.Div as they are
  2. Update math.Min and math.Max to allow 1 or more args, either scalar or slice, or combination of the two
  3. Add math.Sum (allow 1 or more args, either scalar or slice, or combination of the two)
  4. Add math.Product (allow 1 or more args, either scalar or slice, or combination of the two)

This is backwards compatible.

@bep
Copy link
Member

bep commented May 28, 2023

{{ math.Add 6 }} --> error (this is the current behavior, needs > 1 arg)

{{ math.Add (slice 6) }} --> 6

I think this is easier to reason about if you say that if 1 argument and that argument is a slice, then "unpack" it and then apply regular validation rules (which is a way of simulating varg with slice).

With that

{{ math.Add (slice 6) }} => {{ math.Add 6 }} => error (this is the current behavior, needs > 1 arg)
{{ math.Add (slice 6 7) }} => {{ math.Add 6 7 }} => OK.

I'm not sure your examples above ala

{{ math.Min (slice 6) (slice 7) }}

Makes much sense to me. I think we should focus on the main problem in this issue: That it's not currently a way to do math.Add(myslice...) in Go templates (which is a very common construct in Go).

@jmooring
Copy link
Member Author

Sorry, I'm not making myself clear.

{{ $vals := slice }}
{{ range .Pages }}
  {{ $vals := $vals | append .Params.val }}
{{ end }}

{{ math.Sum $vals }}
{{ math.Product $vals }}
{{ math.Min $vals }}
{{ math.Max $vals }}

I don't know how many pages are in the collection. Could be 0, 1, 2, etc. The functions should not error with 1 arg.

@bep
Copy link
Member

bep commented May 28, 2023

To get the discussion back on track. I agree about what you say here:

Leave math.Add, math.Sub, math.Mul, and math.Div as they are
Update math.Min and math.Max to allow 1 or more args, either scalar or slice, or combination of the two
Add math.Sum (allow 1 or more args, either scalar or slice, or combination of the two)
Add math.Product (allow 1 or more args, either scalar or slice, or combination of the two)

@bep bep modified the milestones: v0.113.0, v0.115.0, v0.114.0 Jun 13, 2023
@bep bep self-assigned this Jun 13, 2023
bep added a commit to bep/hugo that referenced this issue Jun 13, 2023
…Product, math.Sum

* Update math.Min and math.Max to allow 1 or more args, either scalar or slice, or combination of the two
* Add math.Sum (allow 1 or more args, either scalar or slice, or combination of the two)
* Add math.Product (allow 1 or more args, either scalar or slice, or combination of the two)

Fixes gohugoio#11030
bep added a commit to bep/hugo that referenced this issue Jun 13, 2023
…Product, math.Sum

* Update math.Min and math.Max to allow 1 or more args, either scalar or slice, or combination of the two
* Add math.Sum (allow 1 or more args, either scalar or slice, or combination of the two)
* Add math.Product (allow 1 or more args, either scalar or slice, or combination of the two)

Fixes gohugoio#11030
bep added a commit that referenced this issue Jun 13, 2023
…Product, math.Sum

* Update math.Min and math.Max to allow 1 or more args, either scalar or slice, or combination of the two
* Add math.Sum (allow 1 or more args, either scalar or slice, or combination of the two)
* Add math.Product (allow 1 or more args, either scalar or slice, or combination of the two)

Fixes #11030
@github-actions
Copy link

github-actions bot commented Jul 5, 2023

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 Jul 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants