korken89 / smlang-rs

A State Machine Language DSL procedual macro for Rust
Apache License 2.0
193 stars 28 forks source link

Refactoring how guard error types are provided #43

Closed ryan-summers closed 1 year ago

ryan-summers commented 1 year ago

This PR refactors how custom guard errors are specified.

Instead of specifying the custom type in the statemachine! macro, the guard error is specified as an associated-type of the trait object.

This makes guard errors more usable for cases when the concrete type of the error is unknown at the declaration of the statemachine (e.g. errors with generic parameters).

I've also removed the derive(Copy, Clone, Eq, PartialEq) for enum Error. There are many types of GuardErrors that would not necessarily implement all of these traits. Since the user is still able to manually implement these traits outside of the declaration, it seems better to be less strict about what types of GuardError are acceptable.

ryan-summers commented 1 year ago

@korken89 / @pleepleus Would either of you be able to review this change? I'd like to use this to release a change to minimq

pleepleus commented 1 year ago

I think I'll have time this week.

Sent from my Verizon, Samsung Galaxy smartphone Get Outlook for Androidhttps://aka.ms/AAb9ysg


From: Ryan Summers @.> Sent: Thursday, September 8, 2022 5:48:45 AM To: korken89/smlang-rs @.> Cc: Donny Zimmanck @.>; Mention @.> Subject: Re: [korken89/smlang-rs] Refactoring how guard error types are provided (PR #43)

EXTERNAL EMAIL - Use caution when responding, clicking, and/or downloading attachments.

@korken89https://urldefense.com/v3/__https://github.com/korken89__;!!J0D89H32QIeNiQ!10D-94qOgKx2d4xBWagSK8iNS_T-1vfVSP01bk6eQzUlJBA6Koyv-gQgNiXLX3g6YYhM0tQfS1LZmwRDlwrXfE5XQfFppg$ / @pleepleushttps://urldefense.com/v3/__https://github.com/pleepleus__;!!J0D89H32QIeNiQ!10D-94qOgKx2d4xBWagSK8iNS_T-1vfVSP01bk6eQzUlJBA6Koyv-gQgNiXLX3g6YYhM0tQfS1LZmwRDlwrXfE4ehLrIBA$ Would either of you be able to review this change? I'd like to use this to release a change to minimq

— Reply to this email directly, view it on GitHubhttps://urldefense.com/v3/__https://github.com/korken89/smlang-rs/pull/43*issuecomment-1240672496__;Iw!!J0D89H32QIeNiQ!10D-94qOgKx2d4xBWagSK8iNS_T-1vfVSP01bk6eQzUlJBA6Koyv-gQgNiXLX3g6YYhM0tQfS1LZmwRDlwrXfE7SZsKKLg$, or unsubscribehttps://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AWHGW3OWIU36KBSDYILG56TV5HOC3ANCNFSM56Y62BPQ__;!!J0D89H32QIeNiQ!10D-94qOgKx2d4xBWagSK8iNS_T-1vfVSP01bk6eQzUlJBA6Koyv-gQgNiXLX3g6YYhM0tQfS1LZmwRDlwrXfE4T_U5E_g$. You are receiving this because you were mentioned.Message ID: @.***>

ryan-summers commented 1 year ago

@korken89 / @pleepleus One more ping on this one if someone has some time to take a look

dzimmanck commented 1 year ago

@ryan-summers, I am checking it out

dzimmanck commented 1 year ago

@ryan-summers,

Looks pretty good to me. I agree that removing the heavy handed derive on the GuardError makes sense. My only feedback is that I think the examples/guard_custom_error.rs file could use some improvement. Most other example files have some code with assertion checks in fn main() {} not only for testing but also to provide some better context to potential users on how a particular feature is useful. However, this was like this even before your PR so I won't gate. If you improve it that would be a bonus.

@MathiasKoch, this does slightly change the syntax for how GuardErrors are used so can cause breaking changes. I think that simply means that this would need to be a 0.6.0 release of code, but I would like your take as it looks like you are the original author of the GuardError feature.

There is also a check fail int he link checker that doesn't make sense to me as I don't see any links you added in the documentation. I will look into that.

MathiasKoch commented 1 year ago

Without having reviewed the actual implementation yet, I think this sounds like a great addition :+1:

dzimmanck commented 1 year ago

Looks like the use of LinkChecker is no longer recommended for GitHub actions in favor of lynchee. I think we should just update the docs.yml file and see if that fixes the tests. I will do as soon as I get a chance unless someone else wants to do it.

dzimmanck commented 1 year ago

@ryan-summers. I create a separate PR #46 to fix the link checker. If that gets accepted and you merge it into this, that should fix your CI tests.

dzimmanck commented 1 year ago

@ryan-summers,

We have a chicken and egg problem in that I appear to be the only active reviewer right now for this project that can approve PRs and I am not allowed to approve my own PR, so my fix to the link checker is not getting merged in.

I think you should just remove the link check in the actions as part of this PR so that the CI tests pass. I will then approve and get this thing moving.

Yatekii commented 1 year ago

Al done :)