Skip to content

Conversation

@erichaney
Copy link
Contributor

No description provided.

@sogaiu sogaiu marked this pull request as draft January 17, 2025 04:07
@sogaiu
Copy link
Collaborator

sogaiu commented Jan 17, 2025

I wonder about the behavior of reduce2 in this example:

(reduce2 + []) # -> nil

May be it could be considered a bug.

reduce2's docstring is currently:

The 2-argument version of reduce that does not take an initialization value. Instead, the first element of the array is used for initialization.

[] has no first element so the intent doesn't seem explicitly specified according to the docstring.


In these kinds of cases, it seems preferable to me to not record these as examples until further clarification so I've marked the PR as a draft.

@sogaiu
Copy link
Collaborator

sogaiu commented Jan 17, 2025

For comparison, reduce in Clojure can be called in two ways:

(reduce f coll) (reduce f val coll)

f should be a function of 2 arguments. If val is not supplied, returns the result of applying f to the first 2 items in coll, then applying f to that result and the 3rd item, etc. If coll contains no items, f must accept no arguments as well, and reduce returns the result of calling f with no arguments.

via: https://clojuredocs.org/clojure.core/reduce

The actual behavior in the following case (2-argument call) seems to match the docstring:

$ clj
Clojure 1.12.0
user=> (reduce + [])
0

May be that was inspired by Common Lisp's reduce as that seems consistent with this:

If the subsequence is empty and no initial-value is given, then the function is called with zero arguments, and reduce returns whatever function does. This is the only case where the function is called with other than two arguments.

via: https://www.lispworks.com/documentation/HyperSpec/Body/f_reduce.htm#reduce

One of the code examples on that page is:

 (reduce #'+ '()) =>  0

Raising an error if the ind (coll) argument is empty seems like another reasonable behavior.

@sogaiu
Copy link
Collaborator

sogaiu commented Jan 17, 2025

It looks like the current behavior is intentional.

I think it might be better to change the docstring or the behavior. Will make an issue.

@sogaiu
Copy link
Collaborator

sogaiu commented Feb 2, 2025

We received clarification in this issue and the docstring has been updated to match the current behavior.

I've made one minor whitespace related edit to one of the files in this PR.


For future readers, let's try to have one file per PR for the examples as mentioned in these steps.

I think it should make review / revision go more smoothly (e.g. if there is more than one file per PR and only one of them has an issue, it can hold up merging of the other content).

@sogaiu sogaiu merged commit 8c76285 into janet-lang:master Feb 2, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants