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

Rewrite macros and tests, add generators #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lynlevenick
Copy link

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.

@lynlevenick lynlevenick force-pushed the recursive_macro_structure branch 2 times, most recently from 906a9ca to 468452e Compare August 12, 2017 20:12
Docs need some more work, but the previous ones were pretty much
just a copy-paste of the tests which seemed like bad form.
@lynlevenick lynlevenick force-pushed the recursive_macro_structure branch from 468452e to 9a9de7a Compare August 12, 2017 20:13
@mattgathu
Copy link
Owner

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.

@mattgathu
Copy link
Owner

Also I'm curious why are some of the previous tests were deleted. Is there a reason for this?

@mattgathu
Copy link
Owner

There's an open issue for generator comprehensions support. May be you can use that for the generator support work.

@lynlevenick
Copy link
Author

lynlevenick commented Aug 14, 2017

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?

@lynlevenick
Copy link
Author

I've opened some more issues that cover the various areas that these changes attend to. #4 #5 #6 #7

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