Skip to content

Add an irrefutable version of the NoSuccess extractor #496

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
Merged
merged 1 commit into from
Jan 25, 2023
Merged

Add an irrefutable version of the NoSuccess extractor #496

merged 1 commit into from
Jan 25, 2023

Conversation

lrytz
Copy link
Member

@lrytz lrytz commented Jan 17, 2023

NoSuccess.unapply cannot be changed due to binary compatibility.

@lrytz lrytz changed the title Add an exhaustive version of the NoSuccess extractor Add an irrefutable version of the NoSuccess extractor Jan 17, 2023
@lrytz lrytz force-pushed the no-success-exhaustive branch from 65ca2c4 to 97d5463 Compare January 17, 2023 10:56
@lrytz lrytz requested a review from dwijnand January 17, 2023 10:56
@lrytz
Copy link
Member Author

lrytz commented Jan 17, 2023

Binary compatibility checks fail because of the new definition, I assume if this goes to a version 2.2.x it should be fine?

@SethTisue SethTisue self-assigned this Jan 19, 2023
@SethTisue
Copy link
Member

I assume if this goes to a version 2.2.x it should be fine

We currently have versionPolicyIntention set to backwards-compat-only BinaryCompatible (rather than to the stricter BinaryAndSourceCompatible for bidirectional compat), so I think we need to take seriously the possibility that MiMa is right to reject the change.

The failure looks like:

[error]  * abstract method NoSuccessE()scala.util.parsing.combinator.Parsers#NoSuccessE# in interface scala.util.parsing.combinator.Parsers is present only in current version
[error]    filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("scala.util.parsing.combinator.Parsers.NoSuccessE")

If I understand ReversedMissingMethodProblem correctly, it's complaining because a method was added to an interface. Doing so isn't backwards binary compatible because existing classes that implement that interface will lack an implementation for the new method.

Must the new extractor be nested inside Parsers?

@lrytz lrytz force-pushed the no-success-exhaustive branch from 97d5463 to 45f437d Compare January 23, 2023 10:01
@lrytz
Copy link
Member Author

lrytz commented Jan 23, 2023

Thanks Seth, I failed to challenge my assumptions...

Added a nested extractor case NoSuccess.I(msg, next), that has better ergonomics as well. Should NoSuccess.unapply be deprecated to help people find the new one?

@SethTisue
Copy link
Member

Um, hmm, judgment call... I'm a bit tempted to say just Scaladoc it rather than deprecate, but really, I'm on the fence. @Philippus opinion?

@dwijnand
Copy link
Member

The exhaustivity already functions as a heads up. So doc it, I agree.

@SethTisue SethTisue assigned lrytz and unassigned SethTisue Jan 24, 2023
@lrytz
Add an irrefutable version of the NoSuccess extractor
cb53ee1 
NoSuccess.unapply cannot be changed due to binary compatibility.
@lrytz lrytz force-pushed the no-success-exhaustive branch from 45f437d to cb53ee1 Compare January 25, 2023 09:09
@SethTisue SethTisue merged commit 537211c into scala:main Jan 25, 2023
@SethTisue
Copy link
Member

@lrytz shall I roll a release?

@lrytz
Copy link
Member Author

lrytz commented Jan 26, 2023

Yes thanks!

@SethTisue SethTisue mentioned this pull request Jan 26, 2023
Closed
@LuciferYang LuciferYang mentioned this pull request Feb 20, 2023
Closed
srowen pushed a commit to apache/spark that referenced this pull request Feb 20, 2023
@LuciferYang @srowen
[SPARK-42489][BUILD] Upgrdae scala-parser-combinators from 2.1.1 to 2…
09d1e94 
….2.0

### What changes were proposed in this pull request?
This pr aims upgrade `scala-parser-combinators from` from 2.1.1 to 2.2.0.

### Why are the changes needed?
scala/scala-parser-combinators#496 add `NoSuccess.I` to helps users avoid exhaustiveness warnings in their pattern matches, especially on Scala 2.13 and 3. The full release note as follows:
- https://github.com/scala/scala-parser-combinators/releases/tag/v2.2.0

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
Pass GitHub Actions

Closes #40083 from LuciferYang/SPARK-42489.

Authored-by: yangjie01 <[email protected]>
Signed-off-by: Sean Owen <[email protected]>
peteraldous pushed a commit to peteraldous/scala-parser-combinators that referenced this pull request Mar 6, 2023
@lrytz @peteraldous
Add an irrefutable version of the NoSuccess extractor (scala#496)
0776ecb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dwijnand dwijnand dwijnand approved these changes

Assignees

@lrytz lrytz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lrytz @SethTisue @dwijnand