8000 Rename Map stepper methods by NthPortal · Pull Request #8095 · scala/scala · GitHub
[go: up one dir, main page]
Skip to content

Rename Map stepper methods #8095

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Rename Map stepper methods #8095

wants to merge 2 commits into from

Conversation

NthPortal
Copy link
Contributor

Rename Map#{key,value}Stepper to
Map#{keys,values}Stepper.

See scala/scala-dev#628 (comment)

@NthPortal NthPortal added this to the 2.13.0-RC3 milestone May 27, 2019
@NthPortal NthPortal requested a review from adriaanm May 27, 2019 20:15
@adriaanm
Copy link
Contributor
adriaanm commented May 27, 2019 •
edited
Loading 
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.MapOps.keyStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.MapOps.valueStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.AbstractMap.keyStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.AbstractMap.valueStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.AbstractMapView.keyStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.AbstractMapView.valueStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.SeqMap#SeqMap4.keyStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.SeqMap#SeqMap4.valueStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.TreeMap.keyStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.TreeMap.valueStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.HashMap.keyStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.HashMap.valueStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.SeqMap#SeqMap3.keyStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.SeqMap#SeqMap3.valueStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.VectorMap.keyStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.VectorMap.valueStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.SeqMap#EmptySeqMap.keyStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.SeqMap#EmptySeqMap.valueStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.SeqMap#SeqMap2.keyStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.SeqMap#SeqMap2.valueStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.SeqMap#SeqMap1.keyStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.SeqMap#SeqMap1.valueStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.mutable.TreeMap.keyStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.mutable.TreeMap.valueStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.mutable.HashMap.keyStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.mutable.HashMap.valueStepper")

other direction:

ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.MapOps.keysStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.MapOps.valuesStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.AbstractMap.keysStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.AbstractMap.valuesStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.AbstractMapView.keysStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.AbstractMapView.valuesStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.SeqMap#SeqMap4.keysStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.SeqMap#SeqMap4.valuesStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.TreeMap.keysStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.TreeMap.valuesStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.HashMap.keysStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.HashMap.valuesStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.SeqMap#SeqMap3.keysStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.SeqMap#SeqMap3.valuesStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.VectorMap.keysStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.VectorMap.valuesStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.SeqMap#EmptySeqMap.keysStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.SeqMap#EmptySeqMap.valuesStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.SeqMap#SeqMap2.keysStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.SeqMap#SeqMap2.valuesStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.SeqMap#SeqMap1.keysStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.SeqMap#SeqMap1.valuesStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.mutable.TreeMap.keysStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.mutable.TreeMap.valuesStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.mutable.HashMap.keysStepper"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.mutable.HashMap.valuesStepper")

@NthPortal
Copy link
Contributor Author

thanks - I forgot about MiMa. I'll update all my PRs

@adriaanm
Copy link
Contributor
adriaanm commented May 27, 2019 •
edited
Loading

No worries, sorry my copy/paste handiwork was not super useful, since you have to split them out into forward/backward. Grepped away a bit too much :)
8000

@NthPortal
Rename Map stepper methods
47018b5 
Rename `Map#{key,value}Stepper` to
`Map#{keys,values}Stepper`.
@NthPortal NthPortal force-pushed the topic/Map-steppers/PR branch from eaa0203 to 47018b5 Compare May 27, 2019 21:09
@NthPortal NthPortal added the library:collections PRs involving changes to the standard collection library label May 27, 2019
@NthPortal
Copy link
Contributor Author

@adriaanm do they get assigned to separate keys?

@dwijnand
Copy link
Member

No, not for sbt's key config.

I guess Adriaan is used to non-prerelease scala/scala dev days where MiMa filters are added to the split https://github.com/scala/scala/blob/2.13.x/src/reflect/mima-filters/2.13.0.backwards.excludes and https://github.com/scala/scala/blob/2.13.x/src/reflect/mima-filters/2.13.0.forwards.excludes files.

@lrytz
Copy link
Member
lrytz commented May 28, 2019

You missed a spot:

scala/src/library/scala/collection/convert/StreamExtensions.scala

Lines 86 to 87 in b616ff0

private type MapOpsWithEfficientKeyStepper[K, V] = collection.MapOps[K, V, CC, _] { def keyStepper[S <: Stepper[_]](implicit shape : StepperShape[K, S]) : S with EfficientSplit }
private type MapOpsWithEfficientValueStepper[K, V] = collection.MapOps[K, V, CC, _] { def valueStepper[V1 >: V, S <: Stepper[_]](implicit shape : StepperShape[V1, S]) : S with EfficientSplit }

I'll take a look at what test case is missing, why the tests didn't catch this.

@lrytz
Copy link
Member
lrytz commented May 28, 2019

Hmm, I wonder, if we rename keyStepper to keysStepper, should we also rename the extension methods asJavaSeqKeyStream to asJavaSeqKeysStream etc?

@lrytz
Copy link
Member
lrytz commented May 28, 2019

I dropped the tests for asJavaParKeyStream / asJavaParValueStream in fa183ff, instead of using a different collection... Pushed a commit to this PR to fix that.

@lrytz lrytz force-pushed the topic/Map-steppers/PR branch from ced2004 to a5b5ba3 Compare May 28, 2019 10:30
8000
@lrytz
Generalize MapOps signature for keyStrem / valueStream extension methods
ee217db 
A sorted map `CC` extends `MapOps[_, _, Map, _]`, not
`MapOps[_, _, CC, _]`, so the extension methods were not available.

Plus some small cleanups.
@lrytz lrytz force-pushed the topic/Map-steppers/PR branch from a5b5ba3 to ee217db Compare May 28, 2019 10:32
@lrytz lrytz requested a review from szeiger May 28, 2019 11:37
@szeiger
Copy link
Contributor
szeiger commented May 28, 2019

My original comment on Slack was: "Another oddity: Map has keysIterator and valuesIterator but keyStepper and valueStepper."

I would prefer consistency but I don't really like keysStepper. Try saying it out loud a few times.

I think keyIterator and valueIterator would be better but do we really want to change them just to remove an extraneous "s"? We copied keySet from Java and keyIterator would be consistent with that. On the other hand, we have keys and values (and Java also has the latter), so keysIterator and valuesIterator make sense when interpreted as keys.iterator and values.iterator.

@adriaanm
Copy link
Contributor

Given autocomplete, names matter a little less. Consistency is nice, but so is constancy :-)

@dwijnand
Copy link
Member

On the other hand, we have keys and values (and Java also has the latter), so keysIterator and valuesIterator make sense when interpreted as keys.iterator and values.iterator

keyIterator returns an iterator of the keys, like keySet returns the set of keys and intStream returns a stream of ints. So for the same reason keyStepper is fine, and better IMO.

@adriaanm
Copy link
Contributor

Ok, let’s keep things as they are and close this one.

@adriaanm adriaanm closed this May 28, 2019
@szeiger
Copy link
Contributor
szeiger commented May 28, 2019

keyIterator returns an iterator of the keys, like keySet returns the set of keys and intStream returns a stream of ints. So for the same reason keyStepper is fine, and better IMO.

Except that we call it keysIterator, not keyIterator

@NthPortal
Copy link
Contributor Author

I guess we migrate to keyIterator in 2.14

@NthPortal NthPortal deleted the topic/Map-steppers/PR branch May 28, 2019 12:43
@adriaanm
Copy link
Contributor

keysiterator flows better than key-iterator
keys-stepper is harder to say than keystepper

@adriaanm
Copy link
Contributor

There is consistency, you just have to chose the right metric :-)

@lrytz
Copy link
Member
lrytz commented May 28, 2019

OK, I'll pull out my bugfixes that I pushed to this PR into a new one.

@dwijnand
Copy link
Member

"Bob, I'm going to need to check this map. Please give me either all the keys or give me a key iterator. Either way, I going to have to check each key one-by-one." Flows, no problem, IMO.

@adriaanm
Copy link
Contributor

Either way, name bikeshedding doesn't really belong in the RC cycle.

@SethTisue SethTisue removed this from the 2.13.0-RC3 milestone May 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adriaanm adriaanm Awaiting requested review from adriaanm

@szeiger szeiger Awaiting requested review from szeiger

Assignees
No one assigned
Labels
library:collections PRs involving changes to the standard collection library
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@NthPortal @adriaanm @dwijnand @lrytz @szeiger @SethTisue
0