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 ok_or and ok_or_else methods #9

Closed
chuckwondo opened this issue Aug 5, 2024 · 8 comments · Fixed by #15
Closed

Add ok_or and ok_or_else methods #9

chuckwondo opened this issue Aug 5, 2024 · 8 comments · Fixed by #15

Comments

@chuckwondo
Copy link
Contributor

I'd like to add ok_or and ok_or_else methods, following Rust's behavior for them:

I realize that this means that this library would then require the rustedpy result library as a dependency. Although both are quite small, I'd be happy to make result an "extras", if you want to allow users to avoid the extra dependency, if they're not using it.

My motivation here is that I have a function that returns a Maybe, but there are cases where I want to convert it to a Result. In particular, this would allow me to make use of it within a result.do call.

For example, if function f returns a Maybe, I can make use of a call to f in a result.do by using ok_or (or ok_or_else), such as in this dummy scenario:

result.do(
   Ok(x + y)
   for x in f(...).ok_or("error message")
   for y in g(x, ...)
)
@francium
Copy link
Member

francium commented Aug 5, 2024

Doing it with the extras approach makes sense for now I think. We can revisit the decision if the two become more interdependent in future, but for now I think that's fine.

You're welcome to push up a PR. Thanks

@chuckwondo
Copy link
Contributor Author

@francium, I see that the github actions in ci.yml are a bit old, so I have some questions:

  1. Would you mind if I updated them to the most recent versions?
  2. Would you mind if I added a dependabot.yml file to automatically bump github action versions periodically (say monthly)?
  3. For any "yes" answers to the above items, do you want me to submit such updates in a separate PR, or may I include them with this one?

@chuckwondo
Copy link
Contributor Author

To avoid cluttering up this PR, I opened a separate issue to address the outdated github actions: #10

I'll address that one first.

@francium
Copy link
Member

francium commented Aug 5, 2024

Yes to all :)

@chuckwondo
Copy link
Contributor Author

@francium, do you have any preference on how you'd like to see things coded to allow for result to be an "extras"?

I'm doing the following, but it seems a bit awkward, so I was wondering if you have any ideas for something better (although this does indeed do the trick):

Near the top of maybe.py:

try:
    import result

    RESULT_INSTALLED = True
except ImportError:  # pragma: no cover
    RESULT_INSTALLED = False

Then within both the Some and Nothing classes, I define the ok_or and ok_or_else methods within a guard, like so (abridged):

class Some(Generic[T]):
    ...

    if RESULT_INSTALLED:
        def ok_or(self, _error: object) -> ...

        def ok_or_else(self, _op: object) -> ...

@francium
Copy link
Member

francium commented Aug 18, 2024

I didn't even know you could do if within a class like that to conditionally define methods. I don't think I've seen that before in any codebase I've read or worked on.

The only alternative I can think of, and I guess I've definitely seen this in real codebases,

class Some(...):
    ...
    def ok_or(...):
        import result
        ...

However, the way you've done it in your example above with a conditional RESULT_INSTALLED in the body to conditionally define the method seems better, even if less elegant, as we keep imports near the top, don't have methods that would (most likely) raise some kind of exception if the result package isn't installed, and magically hide/show those extra methods depending on whether you've got the package installed (although I guess hiding/showing maybe a bit too much magic?...but I've definitely seen a lot of magical behavior in python codebases that leverages the dynamic nature of python)

However, what does make docs do in the case of a conditional within a class/conditionally define methods? Does the docs generator include those methods? Can you check?

Also what about mypy/pyright/ide's/language servers? Do they hide/show those methods correctly depending on if the result package is installed? If not, then maybe we'd want to instead (a) keep imports at the top and (b) raise some kind of exception if the import isn't available to avoid confusion?

@wbolster any opinions on this?

@chuckwondo
Copy link
Contributor Author

@francium, there are a few other bits I didn't include in my earlier comment to keep the comment from being too verbose, but I have managed to get everything to work as expected (tests, linting, docs), whether result is installed or not.

Of course, contributors to this repo should have result installed to make sure all tests are executed and all docs are generated. Without result installed, the tests for the new methods that depend on result are not executed (they are also within an if _RESULT_INSTALLED guard), nor are the related entries generated in the docs.

Regarding the docstrings for the new methods, how would you like to see things noted regarding the need for the result extra to be installed?

Actually, let me submit the PR so you and @wbolster can see everything and make comments on the PR itself. It's probably easier to do that at this point.

@chuckwondo
Copy link
Contributor Author

Also, yes, I've taken care of things working well in the IDE (at least in VSCode, both with mypy and pyright).

chuckwondo added a commit to chuckwondo/maybe that referenced this issue Aug 18, 2024
chuckwondo added a commit to chuckwondo/maybe that referenced this issue Aug 18, 2024
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 a pull request may close this issue.

2 participants