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

2874-clampLowHigh #4682

Merged
merged 7 commits into from Oct 8, 2019

Conversation

@hilaire
Copy link
Contributor

hilaire commented Sep 20, 2019

Fixes #2874

  • implement clampLow:high:,
  • redirect min:max: to it
  • add a test
Fixes #2874
- implement clampLow:high:,
- redirect min:max: to it
- add a test
@Ducasse

This comment has been minimized.

Copy link
Member

Ducasse commented Sep 22, 2019

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

This comment has been minimized.

Copy link
Member

Ducasse commented Sep 22, 2019

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

This comment has been minimized.

Copy link
Member

Ducasse commented Sep 22, 2019

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

@Ducasse

This comment has been minimized.

Copy link
Member

Ducasse commented Sep 22, 2019

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

This comment has been minimized.

Copy link
Member

Ducasse commented Sep 22, 2019

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.

@Ducasse Ducasse requested review from tesonep, estebanlm and guillep Sep 22, 2019
@hilaire

This comment has been minimized.

Copy link
Contributor Author

hilaire commented Sep 22, 2019

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

This comment has been minimized.

Copy link
Contributor Author

hilaire commented Sep 22, 2019

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

This comment has been minimized.

Copy link
Member

Ducasse commented Sep 22, 2019

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

@Ducasse

This comment has been minimized.

Copy link
Member

Ducasse commented Sep 25, 2019

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

This comment has been minimized.

Copy link
Contributor Author

hilaire commented Sep 25, 2019

What is the expect result in these scenari

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

hilaire added 2 commits Sep 25, 2019
Additional comments to min:max:
@Ducasse

This comment has been minimized.

Copy link
Member

Ducasse commented Sep 25, 2019

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

This comment has been minimized.

Copy link
Contributor Author

hilaire commented Sep 26, 2019

I don't understand the conflict

@hilaire

This comment has been minimized.

Copy link
Contributor Author

hilaire commented Sep 28, 2019

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

This comment has been minimized.

Copy link
Member

Ducasse commented Sep 28, 2019

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

This comment has been minimized.

Copy link
Contributor Author

hilaire commented Sep 28, 2019

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

@Ducasse

This comment has been minimized.

Copy link
Member

Ducasse commented Sep 29, 2019

Sorry which other point.

@hilaire

This comment has been minimized.

Copy link
Contributor Author

hilaire commented Oct 1, 2019

in my previous comment

@Ducasse

This comment has been minimized.

Copy link
Member

Ducasse commented Oct 8, 2019

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

@Ducasse Ducasse closed this Oct 8, 2019
@Ducasse Ducasse reopened this Oct 8, 2019
@Ducasse

This comment has been minimized.

Copy link
Member

Ducasse commented Oct 8, 2019

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

Ducasse added 3 commits Oct 8, 2019
@Ducasse Ducasse requested review from Ducasse and removed request for tesonep, estebanlm and guillep Oct 8, 2019
@Ducasse Ducasse merged commit 5297491 into pharo-project:Pharo8.0 Oct 8, 2019
2 of 3 checks passed
2 of 3 checks passed
probot/minimum-reviews Pending review approvals
WIP Ready for review
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.