cute
cute copied to clipboard
mattgathu
Rewrite macros and tests, add generators
Docs need some more work, the previous ones were almost a copy-paste of the tests which seemed like bad form. They also didn't pass cargo test, so I added harnesses to make them pass properly, and updated the README to include the documentation as updated from the start of the file.
The macros themselves could use some documentation.
Hi @JustANull thanks a lot for this PR, that's a lot of work!! I like what you've done but I can't merge it since the PR has a lot going and does not meet the criteria below:
- Make it small
- Do only one thing
- Avoid re-formatting
- Make sure all tests pass
I think, a good approach would be to submit several PRs, each having a single responsibility. It would be easy to reason about them and make merging straight forward. Checkout this awesome guide.
Let me how this sounds to you. I'm happy to help you break up this PR into smaller ones.
Also I'm curious why are some of the previous tests were deleted. Is there a reason for this?
There's an open issue for generator comprehensions support. May be you can use that for the generator support work.
This got a bit out of hand because it basically ends up as a complete rewrite of the original macros and tests. I can't really break it up into multiple PRs easily because all of the code is shaped by the c! (and i!) macros, there aren't well-defined units to make each change in.
Originally, the docs didn't wrap the examples with # hidden code to allow cargo test to pass, and they were also practically a copy-paste of the test cases? Removing all of them was too much, I agree, but if I wanted to provide examples for generators in the same sort of format all of the docs would be duplicated.
The original c! macro had inconsistent ordering on which for clause ended up inside which other for clause. Unfortunately, the tests tested for the inconsistent behavior. The rewrite fixes that while letting conditions happen at any level, and any number of times.
Adding the i! macro with identical syntax under the original testing structure would have required duplicating all of the tests and replacing c! with i! in each of them, which felt very copy-pastey and destined to not be updated in parallel.
Finally, under the new macro structure that supports c! and i! (a token-tree muncher), it isn't as simple as testing all of the possible syntax variations as in the original tests. The tests should likely be updated to more thoroughly cover the branches of the new macros, which I think the new tests do?
I've opened some more issues that cover the various areas that these changes attend to. #4 #5 #6 #7