2874-clampLowHigh

[hilaire]

Fixes #2874

• implement clampLow:high:,
• redirect min:max: to it
• add a test

[Ducasse]

Could we improve the method comment:

Magnitude >> clampLow: minValue high: maxValue [
"Answer the value within the interval [minValue ; maxValue]; if the value is contained into the interval then return it, else return the adequate interval value."

[Ducasse]

I read the translation in french about clamp (fixer, attacher) and I was thinking that clampLow:high: is not a super nice name.
We could really take the time to think about a better.
May be we can ask in the ML for ideas.
I would add between
and I would prefer beBetween: and:
Because clampLow:high: does not convey the intention of the method (at least when I read it, I do not understand it).

[Ducasse]

" 10 clampLow: 12 high: 20 >>> 12"
should be rewritten "(10 clampLow: 12 high: 20) >>> 12"

[Ducasse]

Hi hilaire

I was thinking about it and I did not like the solution.
Changing min:max: to introduce a new method with its own problems could be better.
I think that it would be great that the new method is not depend on the fact that min is smaller than max.

Because I understand the original point: it was that with low: and high: people would pass "correct" i.e., argument in order smaller and larger to get a positive interval.
But this only works well when we pass number now if we pass x and y.
We have not idea that the result will not be the one we expect.

I would like to have a real discussion about it.
And I would let min:max: like it is but adding a real method comment and examples.

Then I would define beBetween: that works in all cases for real.
What do you think?

Here are the scenario.

(5  min: 12 max: 20) 
>> 20
>>> this is a problem 
(5 clampLow: 12 high: 20) 
>> 12
(5 beBetween: 12 high: 20)
>> 12 

(5  min: 20 max: 12)
>> 12
(5 clampLow: 20 high: 12)
>> 20 ????
(5 beBetween: 20 high: 12)
>> 12

(100  min: 20 max: 12) 
>> 20
(100 clampLow: 20 high: 12)
>> 20

(100  min: 12 max: 20) 
>> 20
(100 clampLow: 12 high: 20) 
>> 20

[Ducasse]

min:max: comment should really say that it is min:thenMax:
and may be we should rename it like.

The solution would be

• min:max: -> renamed min:thenMax:
  add
• beBetween:and:

with different semantics.

[hilaire]

Agree, I don't like much the clampLow:hight: message. The intention was the keep come compatibility with Squeak, but we likely passed this point.

[hilaire]

May be to keep some compatibility issues, we could just keep min:max: as it is, renamed its parameters, then add a proper comment plus test as you suggested it.
As least the user will have proper information.

Now thinking about it, beBetween:and: when self is outside the interval, the message name is not meaningful about which result we expect.

10 beBetween: 5 and: 8
=> 5 or 8 ?
10 beBetween: 8 and: 5
=> 5 or 8 ?

Or something like beBetweenLow:andHigh: but your idea is the parameters can be reversed without harm, and it will not work with this name.

It fell like antinomism.

We could just live with min:max:

[Ducasse]

I will discuss with pablo and guille and let you know.

[Ducasse]

After discussing with guile and pablo I think that we can

• deprecate min:max: into min:thenMax:
• then we can introduce add beBetween:and: with a correct semantics (not having the trouble of mix:max:)

[hilaire]

What is the expect result in these scenari

10 beBetween: 5 and: 8
=> 5 or 8 ?
10 beBetween: 8 and: 5
=> 5 or 8 ?

[Ducasse]

Like clipping 8 because 8 is the closest.
May be we should discuss on the mailing-list if it is worth now
to me clampLow:High: does not bring anything and it is more confusing than min:thenMax:

Stef

[hilaire]

I don't understand the conflict

[hilaire]

I am not sure either about replacing min:max: by min:thenMax:

• it will brake a lot of packages, it is use in many places
• the intend of the message min:thenMax: is only slightly better (it tells about what it is doing but not about its meaning), indeed, the parameters are still swapped in meaning (knots in your brain).
• a min:max: message with proper comments and examples will already help a lot the developer

[Ducasse]

Hilaire usually for central message we deprecated them over multiple version.
So we can really introduce min:thenMax: and fix all the senders of min:max: to use mix:thenMax: and then we deprecate with transformation min:Max: and people get two years to automatically migrate.
I think that this is more than effort.

[hilaire]

@Ducasse ok understood. What do you think about the other points I mentioned?

[Ducasse]

Sorry which other point.

[hilaire]

in my previous comment

[Ducasse]

I do not understand how to read the conflicts else the code is good for me.

[Ducasse]

I fixed the conflicts and waiting for the build to rerun before integrating the changes.