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

match syntax fails when used with raw Result instead of Failure/Success #56

Closed
3 tasks done
Artalus opened this issue May 10, 2023 · 3 comments · Fixed by #57
Closed
3 tasks done

match syntax fails when used with raw Result instead of Failure/Success #56

Artalus opened this issue May 10, 2023 · 3 comments · Fixed by #57
Labels
bug Something isn't working documentation Improvements or additions to documentation
Milestone

Comments

@Artalus
Copy link

Artalus commented May 10, 2023

First Check

  • I added a very descriptive title to this issue.
  • I used the GitHub search to find a similar issue and didn't find it.
  • I already read and followed all the documentation and didn't find an answer.

Description

Usage > Detail docs mention this:

If you are using Python 3.10 or above, you can take advantage of new syntax proposed in PEP 636 – Structural Pattern Matching to handle the result.

from meiga import Success, Failure, Error

class NoSuchKey(Error): ...
class TypeMismatch(Error): ...

match result:
    case Success(_):
        print(f"Success")
    case Failure(NoSuchKey()):
        print("Failure with NoSuchKey")
    case Failure(TypeMismatch()):
        print("Failure with TypeMismatch")
    case _:
        print("default")

While mostly convenient, this code does not work when the result variable was constructed not with Failure(NoSuchKey()) but with Result(failure=NoSuchKey()):

In [1]: from meiga import Result, Failure, Error, Success
In [2]: class NoSuchKey(Error): pass
In [3]: result = Result(failure=NoSuchKey())
In [4]: match result:
   ...:     case Success(_):
   ...:         print(f"Success")
   ...:     case Failure(NoSuchKey()):
   ...:         print("Failure with NoSuchKey")
   ...:     case Failure(TypeMismatch()):
   ...:         print("Failure with TypeMismatch")
   ...:     case _:
   ...:         print("default")

default

Getting rid of error types in case statements does not help:

In [5]: result = Result(failure=Error())
In [6]: match result:
   ...:     case Success(_):
   ...:         print("Success")
   ...:     case Failure(_):
   ...:         print("Failure")
   ...:     case _:
   ...:         print("default")

default

match and its magics are too new tech for me, so I cannot provide any insight on why isn't it working or what could be amended.

Operating System

Linux

Operating System Details

No response

meiga Version

1.8.3

Python Version

python --version

Additional Context

No response

@Artalus Artalus added the bug Something isn't working label May 10, 2023
@acostapazo
Copy link
Collaborator

Hi 👋🏾

First of all, thanks again for your valuable feedback :)

Context

Success and Failure are Result are typed superclasses.
Please, check its implementation in:

To make available the PEP 636, you have to add to your classes the __match_args__ (check doc in https://peps.python.org/pep-0622/#special-attribute-match-args).

In typed aliases we add specific value for success and failure depending on its semantic:

This is why match pattern is working when you use the alias Success and Failure.

What happens when the Result class is used to define the return?

The match pattern will use the __match_args__ of the Result base class (https://github.com/alice-biometrics/meiga/blob/main/meiga/result.py#L19). This class as you can check have two match arguments (success value and failure value)

__match_args__ = ("_value_success", "_value_failure")

So, you have to match your cases with the Result match arguments.

Following your example:

from meiga import Result, Failure, Error, Success
class NoSuchKey(Error): pass
result = Result(failure=NoSuchKey())
match result:
    case Result(str(), _):
        print(f"Success")
    case Result(_, NoSuchKey()):
        print("Failure with NoSuchKey")
    case Result(_, TypeMismatch()):
        print("Failure with TypeMismatch")
    case _:
        print("default")

This will print Failure with NoSuchKey

And

result = Result(failure=Error())
match result:
    case Result(str(), _):
        print("Success")
    case Result(_, Error()):
        print("Failure")
    case _:
        print("default")

will print also Failure as expected.

This way is less convenient than using the aliases (Success and Failure) and forces us to define the type of the success value for the first case of the examples (case Result(str(), _)).

I don't know if there is a way of improving the dev experience of matching cases when uses Result base class. Any idea?

On the other hand, we strongly recommend the adoption of Success and Failure alias to improve the semantic of your code.

@Artalus
Copy link
Author

Artalus commented May 11, 2023

Kudos to you for taking the time to explain it in detail 👍

I don't know if there is a way of improving the dev experience of matching cases when uses Result base class. Any idea?

  • Is it possible to make match look only at the values in __match_args__, ignoring the class of object itself? So that from matches point of view Result objects would be identical to Success, Failure and Whatever as long as some inner invariant stands?
  • Maybe typing.NewType or some other type hints might help somehow, so instead of subclassing Result we would have just some convenience wrapper named Success, like it was with a function before?
  • Maybe reverse it top to bottom? Turn public Result class into a private _Result one - and instead have Result[X, Y] as some type-hint abstraction, like Success[X] | Failure[Y]?..

On the other hand, we strongly recommend the adoption of Success and Failure alias to improve the semantic of your code.

In that case I would suggest removing raw Result examples from docs, or restructuring it in a way that would make it clearer that Success("x") is a way to go instead of Result(success="x"). Maybe even providing some diagnostic that would make an IDE hint you "do not construct Result objects yourself!".

Right now I land at Getting started and the first things it says me are "new type class, the Result[Type, Type]" (which is mostly fine, as I do have to use Result in return types) and "basic example: return Result(failure=NoSuchKey())". Aliases are then presented as an additional feature.

@acostapazo
Copy link
Collaborator

From the easiest to the most difficult.

In that case I would suggest removing raw Result examples from docs, or restructuring it in a way that would make it clearer that Success("x") is a way to go instead of Result(success="x"). Maybe even providing some diagnostic that would make an IDE hint you "do not construct Result objects yourself!".
Right now I land at Getting started and the first things it says me are "new type class, the Result[Type, Type]" (which is mostly fine, as I do have to use Result in return types) and "basic example: return Result(failure=NoSuchKey())". Aliases are then presented as an additional feature.

Agree.

Although it is referred to in the documentation, it may be necessary to be more explicit.

Screenshot 2023-05-11 at 15 33 24

match issue

We have tested some of these options, but in some of these combinations they have problems in the typing. The best option found was the Success and Failure superclasses. This makes it very convenient for the match case and also follows a syntax similar to Results of other languages (e.g. https://github.com/antitypical/Result#use).

@acostapazo acostapazo added the documentation Improvements or additions to documentation label May 11, 2023
@acostapazo acostapazo added this to the v1.9.0 milestone May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants