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 with state - fixes #121 #133

Merged
merged 5 commits into from
Apr 1, 2023
Merged

Conversation

panoukos41
Copy link
Contributor

Hey @domn1995 I have created an implementation that should provide a solution for #121 that generates Variant & Specific Match methods that accept custom state eg:

// Variant
public abstract TMatchOutput Match<TState, TMatchOutput>(TState state, ...);
public abstract void `Match<TState>(TState state, ...);

// Specific
public abstract TMatchOutput MatchSpecific<TState, TMatchOutput>(TState state ...);
public abstract void MatchSpecific<TState>(TState state ...);

I am also creating this as a draft since I would like some guidance about the tests and implementation.

For the implementation: I implemented the state methods together with the non-state ones but I could also move them to a different method. I just thought they belong together with their non-state counterparts.

For the tests: I know I could change some tests that execute methods to also execute the state counterpart or I could create a new test class that does just that but I am not sure which way to go 😄

Add `Match<TState, TMatchOutput>` and `Match<TState>` for variant and specific methods which accept a `state` object and pass it to the provided functions.
Refactor the method to generate properly for an empty union and ensure all current tests are passing.
@domn1995
Copy link
Owner

Thanks for the contribution! I'll take a look at this later today :)

@domn1995
Copy link
Owner

domn1995 commented Mar 30, 2023

For the implementation: I implemented the state methods together with the non-state ones but I could also move them to a different method. I just thought they belong together with their non-state counterparts.

I think this works well right now. The source builder is getting pretty long, but we can clean that up as a separate pull request later 👍

For the tests: I know I could change some tests that execute methods to also execute the state counterpart or I could create a new test class that does just that but I am not sure which way to go

My gut says to split it up into its own "match with state" test class. This way, we don't couple any of the existing test suites to the new one. Let me know if you need any help with those tests. :)

@panoukos41
Copy link
Contributor Author

Thanks for your fast response and sorry for not getting back quickly It was midnight here in Greece 🇬🇷.

I will give it a try later today and if I have trouble finding which tests to mimic I will let you know 😁

Add tests for `Variant` and `Specific` methods with `state` object.
@panoukos41
Copy link
Contributor Author

@domn1995 I pushed the tests let me know if something needs changing or how I could help more 😄 otherwise :shipit:! (always wanted to use this emoji :D)

@panoukos41 panoukos41 marked this pull request as ready for review March 31, 2023 17:10
@domn1995
Copy link
Owner

domn1995 commented Apr 1, 2023

Everything look awesome! Thanks again for the contribution! :shipit: :shipit: :shipit:

@domn1995 domn1995 merged commit c2e60c2 into domn1995:main Apr 1, 2023
@panoukos41 panoukos41 deleted the match-with-state branch April 1, 2023 08:31
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