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

Add mapTry to Option #2950

Closed
wants to merge 6 commits into from
Closed

Add mapTry to Option #2950

wants to merge 6 commits into from

Conversation

bvkatwijk
Copy link
Contributor

  • Add mapTry method to Option, which is a shortcut for .toTry().mapTry
    • I propose this because when mapping an optional using a checked method, I find it intuitive that the type changes to a Try, and mapTry methods exist on other types already
    • I initially added it onto Value but this ran into an issue where a method with same name but return value already exists on subtypeFuture

Unrelated but maybe helpful (I can remove it if desired)

  • Add toolchain resolver
    • After forking the project I could not load it in IntelliJ, Gradle complained that it could not resolve the toolchain. Added a resolver as per the Gradle Docs
  • Add Nested test class for mapTry tests instead of a comment
    • This achieves organizational separation just like the comment, but now JUnit also understands it and can e.g. run only these tests in isolation.

@bvkatwijk bvkatwijk requested a review from pivovarit as a code owner December 29, 2024 10:54
@pivovarit
Copy link
Member

Thanks for your contribution :) looking good!

Add Nested test class for mapTry tests instead of a comment

makes sense - we've migrated to JUnit5 recently, but we're not leveraging it fully

After forking the project I could not load it in IntelliJ, Gradle complained that it could not resolve the toolchain.

sounds good - could you add it a a separate change, though? so that we don't mix two things together

I initially added it onto Value but this ran into an issue where a method with same name but return value already exists on subtypeFuture

that's something we might want to straighten out before releasing 1.0.0

This reverts commit 89c20aa.
@bvkatwijk
Copy link
Contributor Author

Proposing to migrate to nested classes in #2951

Copy link

codecov bot commented Jan 3, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (version/1.x@82bd4ed). Learn more about missing BASE report.

Additional details and impacted files
@@              Coverage Diff               @@
##             version/1.x    #2950   +/-   ##
==============================================
  Coverage               ?   92.72%           
  Complexity             ?     5263           
==============================================
  Files                  ?       89           
  Lines                  ?    12553           
  Branches               ?     1598           
==============================================
  Hits                   ?    11640           
  Misses                 ?      725           
  Partials               ?      188           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bvkatwijk bvkatwijk mentioned this pull request Jan 10, 2025
@bvkatwijk bvkatwijk closed this Jan 10, 2025
pivovarit pushed a commit that referenced this pull request Jan 12, 2025
Add `mapTry` method to Option, which is a shortcut for
`.toTry().mapTry`:
* when mapping an optional using a checked method, I find it intuitive
that the type changes to a Try, and `mapTry` methods exist on other
types already
* I initially added it onto `Value` but this ran into an issue where a
method with same name but return value already exists on subtype`Future`
  
----

Rebase of #2950
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants