Skip to:

  1. Skip to Jira Navigation
  2. Skip to Side Navigation
  3. Skip to Main Content

{min,max}-key return first match in >= 1.9.0-alpha20, used to return the last one

Description

The https://clojure.atlassian.net/browse/CLJ-99#icft=CLJ-99 fix, first included in 1.9.0-alpha20, changes behaviour of min-key and max-key to return the first argument among those with the minimum/maximum key, whereas in previously they both returned the last matching argument:

Clojure 1.9.0-alpha19 user=> (apply min-key :x [{:x 1000} {:x 1001} {:x 1002} {:x 1000 :second true}]) {:x 1000, :second true}
Clojure 1.9.0-beta1 user=> (apply min-key :x [{:x 1000} {:x 1001} {:x 1002} {:x 1000 :second true}]) {:x 1000}

Arguably the new behaviour is more natural, however this is a regression from prior behavior.

Proposed: Restore the original behavior and update docstring.
Patch: 0002-CLJ-2247-document-last-match-semantics-of-min-max-ke.patch
Screened by: Alex Miller

Environment

None

Attachments

2
  • 05 Oct 2017, 09:13 AM
  • 04 Oct 2017, 09:20 PM

Activity

Show:

Michał Marczyk October 5, 2017 at 9:13 AM

That's fair enough re: documenting the behaviour. Here's a patch that applies on top of the 0001 patch and only tweaks the docstrings by adding "If there are multiple such xs, the last one is returned."

Driving the behaviour using extra arguments is not really an option in this case, as min-key and max-key take varargs.

An extra pair of functions along the lines of min-key-first / max-key-first could be contemplated (as could min-key-last / max-key-last to provide a migration path for existing users if the new behaviour were kept). Whether it should is a separate question which it may be ok to address after the release.

Bozhidar Batsov October 5, 2017 at 7:38 AM

I also think that the expected behaviour should be mentioned in the documentation, as I'd certainly expect this to behave like it does after the patch. Perhaps down the road we can make this configurable with an extra parameter?

Alex Miller October 4, 2017 at 9:54 PM

Agreed on restoring original behavior.

Michał Marczyk October 4, 2017 at 9:38 PM

As a side note, the original behaviour arose from the use of reduce in the implementation of both functions:

Clojure 1.9.0-beta1 user=> (apply min-key :x [{:x 1000} {:x 1001} {:x 1002} {:x 1000 :second true}]) {:x 1000} user=> (reduce #(min-key :x %1 %2) [{:x 1000} {:x 1001} {:x 1002} {:x 1000 :second true}]) {:x 1000, :second true}

Michał Marczyk October 4, 2017 at 9:20 PM

Restoring the original behaviour (option 2) seems like a reasonable default position, particularly so close to a major release, so here's a patch that does that.

Completed

Details

Assignee

Reporter

Labels

Approval

Ok

Patch

Code

Priority

Affects versions

Fix versions

Created October 4, 2017 at 9:17 PM
Updated October 6, 2017 at 7:47 PM
Resolved October 6, 2017 at 7:47 PM