JoshMcguigan / multi_try

Safely combine results
https://www.joshmcguigan.com/blog/multi-try-improved-validation-logic-rust/
Apache License 2.0
81 stars 3 forks source link

Optional manual implementation as a feature flag #3

Open Nerglej opened 2 months ago

Nerglej commented 2 months ago

Hello!

I really, really like this library, so I made a few tweaks that I needed for a project, even though this project is pretty old by now.

I made a couple of changes:

When enabling the manual feature, a simple call to the macro is removed from the library. It then needs to be called, as explained in "Manual implementation" in the docs. Since it's a feature-flag, no tests are broken, except for the doc-tests. I decided to ignore this doc-test instead, because it's simple. If anyone has an idea to make it compile, you're very welcome!

I also added a bit of documentation to the other macros that got exported, to warn users, because of limits with rusts macros, when being exported to other crates.

Hope this PR isn't for no use! Have a nice day😊

JoshMcguigan commented 2 months ago

Thank you for the contribution! If I'm understanding the change properly, this implements:

Are these two features in conflict? It seems like a user might enable the manual feature to keep the code size minimal, but then be forced to use the recursive macro implementation which would increase their code size.

As for the manual feature, I think other crates solve this by exposing features like multi-try-2, multi-try-3, .., multi-try-26. That was we don't have to expose the implementation of the MultiTry trait to the user, even if it's abstracted by a macro.

Since it's a feature-flag, no tests are broken, except for the doc-tests. I decided to ignore this doc-test instead, because it's simple. If anyone has an idea to make it compile, you're very welcome!

I think this is breaking the doc tests because the doc test has multiple calls to impl_multi_try, which is no longer allowed as of this change (since impl_multi_try does a recursive implementation now by default). Perhaps this is more reason to remove the recursive implementation?