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

Added Result merge method #225

Merged
merged 1 commit into from
Aug 29, 2024

Conversation

brendanmaguire
Copy link
Contributor

Also:

  • is_ok and is_error use a comparison instead of a match as the comparison is faster
  • Fixed a typo

@dbrattli
Copy link
Owner

Hi, thanks for the PR. Do you have any reference for the merge operator? Who else is using it? I would prefer operators like orElse and orElseWith, ref: https://demystifyfp.gitbook.io/fstoolkit-errorhandling/fstoolkit.errorhandling/result/orelsefunctions#result.orelsewith

@brendanmaguire
Copy link
Contributor Author

brendanmaguire commented Aug 28, 2024

Hi, thanks for the PR.

Happy to contribute 🙂

Do you have any reference for the merge operator? Who else is using it?

There is an Either#merge in Scala (docs). (Not sure of your familiarity with Scala but Either is pretty much the same as Result)

I would prefer operators like orElse and orElseWith, ref: https://demystifyfp.gitbook.io/fstoolkit-errorhandling/fstoolkit.errorhandling/result/orelsefunctions#result.orelsewith

merge is orthogonal to these imo. An example use of merge would be if you get a Result from your service layer and want to convert it to a HTTPResponse to return from the router layer.

Simple example:

def create(customer_id: str, email: str) -> HTTPResponse:
    service_result: Result[Customer, InvalidEmailAddress] = service.create_customer(customer_id, email)
    response: HTTPResponse = service_result.map(customer_to_response).map_error(invalid_email_to_response).merge()
    return response

I think the above code might also benefit from a Either#bimap (Scala cats docs) if you would be open to that addition? I tend to use bimap and merge together quite often in Scala.

Example (removing the extra variables from the above which I have included to highlight the types):

def create(customer_id: str, email: str) -> HTTPResponse:
    return (
        service.create_customer(customer_id, email)
        .bimap(customer_to_response, invalid_email_to_response)
        .merge()
    )

@brendanmaguire
Copy link
Contributor Author

I've created a PR to add the or_else* methods too #226

@dbrattli
Copy link
Owner

Sorry I think I made a merge conflict with your PR. Anyways, I see that merge is the same as default_with(lambda x: x)

Also:
* `is_ok` and `is_error` use a comparison instead of a match as the comparison is faster
* Fixed a typo
Copy link
Owner

@dbrattli dbrattli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks for contributing this

@dbrattli dbrattli merged commit 172bb4b into dbrattli:main Aug 29, 2024
3 checks passed
@brendanmaguire brendanmaguire deleted the result-merge-method branch August 29, 2024 09:18
@brendanmaguire
Copy link
Contributor Author

Nice catch on the default_with. Pretty succinct now 🙂

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