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

Improved case macro #31

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

LasterAlex
Copy link

Inspired by this message.

Although the code for case! was fully rewritten, there are no breaking changes!

The goal was to make the case! pattern matching the same as the rust pattern matching, and I think it's pretty close for dptree purposes!

Sad that it's almost impossible to infer the type of None, but it's fine

I think the tests cover all of the new use cases

@hirrolot hirrolot self-requested a review November 7, 2024 23:52
Copy link
Collaborator

@hirrolot hirrolot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for an inexcusably late review. I find it extremely repulsive to read advanced macro_rules! code like this one, although it's absolutely not your fault.

The tests look fine at first glance, but I'll need to scrutinize them once more.

If you're out of resources right now, I can make the changes. Let me know if you can address my review suggestions at the moment.

src/lib.rs Outdated
Comment on lines 91 to 92
/// You can also go with even more complexity and write a syntax similar to rusts pattern matching
/// system:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like this description to be more formal. For example, the first bullet point doesn't show whether this pattern can be used like Enum::MyVariant(SecondEnum::MyVariant1, SecondEnum::MyVariant2), and same for the other points.

src/lib.rs Outdated Show resolved Hide resolved
///
/// It's recursive, so on every parameter you can insert a new struct, tuple struct, enum variant or
/// anything else.
///
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the examples below, we should add at least one (simple) example demonstrating the new functionality you're adding.

@LasterAlex
Copy link
Author

My pc is currently in repair, so i physically cant make the changes until it is done, which should happen soon enough
The code looks awful, yes, and it really is a good question whether this should be the default, or an experimental feature
Ill get on this when i have some time, but feel free to commit changes as you see fit

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