Mark use of BidirectionalIterator as deprecated.
#106471
Conversation
We are deprecating the interface in the Dart SDK (https://dart-review.googlesource.com/c/sdk/+/249486) because it simply doesn't carry its own weight. There is no existing use of the interface as an abstraction (no function expecting a `BidirectionalIterator`). Almost every class which currently implement `BidirectionalIterator` (and there are very few of those) also extend the interface with more methods, so users will be using the objects at that subclass ttpe instead. This is the case here, everyone uses `VerticalCaretMovementRun`, nobody ever casts it to `BidirectionalIterator`. Some classes which can move a cursor in both directions choose a different name than `movePrevious` insteed. And because of the above, that doesn't ever matter. And there are very few uses at all. So, interface will go away, and nobody will be poorer for it. I'd actually recommend, instead this change, to just make the class extend `Iterable` directly and drop `@override` from `movePrevious`. It's technically a breaking change, because the class is no longer assignable to `BidirectionalIterator`, but nobody ever does that anyway.
flutter-dashboard
bot
added
the
framework
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
There is only one use of it in the SDK, `RuneIterator`, one use of it outside, `TreeSet` from `package:quiver`, and one known use in Google internal code. There are other bi-directional iteration interfaces, like `CharacterRange` from `package:characters`, which can move a cursor in two directions, but which didn't loke the `movePrevious` name and therefore are not `BidirectionalIterator`s. (There is also a Google internal class which mentions explicitly why they're not `BidirectionalIterator`.) There is no real need for this "shared" interface. It doesn't carry its own weight. We have no abstractions which work on the *interface*, only code that works on one specific concrete implementation. I'd recommend: * `quiver` introduce a `TreeSetIterator` for their use-case (the actual class has more public members than `BidirectionalIterator`, which cannot be accessed through the declared type.) * the Google internal code introduces its own interface (which is just `Iterable` + `movePrevious`. * The SDK will just make `RuneIterator` not implement `BidirectionalIterator` at some (breaking) point. Maybe just when we remove the type. FLUTTER: Needs to land flutter/flutter#106471 before merging this CL into Flutter. Change-Id: Iaaa6b0c428d30eb3b68898c077d265ac0a622805 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/249486 Reviewed-by: Nate Bosch <nbosch@google.com> Commit-Queue: Lasse Nielsen <lrn@google.com>
I agree that this is the right change. Could you just apply that in this PR? |
Just extend `Iterator` and remove `@override` from `movePrevious`.
|
Done, PTAL. |
| @@ -186,7 +186,6 @@ class VerticalCaretMovementRun extends BidirectionalIterator<TextPosition> { | |||
| return true; | |||
| } | |||
|
|
|||
| @override | |||
There was a problem hiding this comment.
This method now needs a doc comment since it is no longer inheriting the docs from the super class. Adding one should make the checks happy.
|
Looks like there's also a merge conflict that requires resolving. |
|
It seems like someone has already landed the same change, with documentation, on the master branch. So, yey! My job here is done 😁. |
Marks the one use of
BidirectionalIteratorin the repo to ignore the class becoming deprecated.We are deprecating the interface in the Dart SDK (https://dart-review.googlesource.com/c/sdk/+/249486) because it simply doesn't carry its own weight.
There is no existing use of the interface as an abstraction (no function expecting a
BidirectionalIterator). This shows that it's not an abstraction the people need. Individual classes can have amovePreviouswithout them needing a common super-interface.Almost every class which currently implement
BidirectionalIterator(and there are very few of those) also extend the interface with more methods, so users will be using the objects at that subclass type instead.This is the case here, everyone uses
VerticalCaretMovementRun, nobody ever casts it toBidirectionalIterator. It doesn't matter that it implementsBidirectionalIterator.Some classes which can move a cursor in both directions choose a different name than
movePreviousinstead.And because of the above, that also doesn't ever matter.
There are very few uses at all, and they are not necessary. The interface will go away, and nobody will be poorer for it.
I'd actually recommend, instead this change, to just make the class extend
Iterabledirectly and drop@overridefrommovePrevious. It's technically a breaking change, because the class is no longer assignable toBidirectionalIterator, but nobody ever does that anyway. But I'll let you decide when to do that (as long as it's before we remove the interface).