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

Add labels! and labels_sep_by! utility macros. #8

Merged
merged 1 commit into from
Jun 5, 2016

Conversation

dashed
Copy link
Contributor

@dashed dashed commented Jun 4, 2016

Closes #7

I use suggestion from #7 (comment)


I added labels_sep_by! because I wanted to be able to customize delimiters. I use it to generate the style attribute of the element.


@Stebalien I couldn't get this to work labels!("active" if false) since:

expr and stmt variables may only be followed by one of: => , ;

according to https://doc.rust-lang.org/book/macros.html

Any suggestions?

@dashed
Copy link
Contributor Author

dashed commented Jun 4, 2016

I realized I forgot to create a branch. 😓

@Stebalien
Copy link
Owner

Nice!

expr and stmt variables may only be followed by one of: => , ;

according to https://doc.rust-lang.org/book/macros.html

Any suggestions?

😢 I was thinking of patterns. Oh well. => is clear enough.

($sep:expr; $item:expr) => {{

use $crate::Template;
use $crate::RenderOnce;
Copy link
Owner

Choose a reason for hiding this comment

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

Instead of importing and calling, just use the fully qualified syntax: $crate::RenderOnce::render(&$item, tmpl)

Otherwise, you run the risk of running into conflicting functions.

@Stebalien
Copy link
Owner

Just a few nits and this looks good to go.

@dashed
Copy link
Contributor Author

dashed commented Jun 4, 2016

I amended with documentation and suggested refactor.

I also refactored the macros to add separator only if there is a previous label. This was an edge case I missed.

@dashed dashed force-pushed the master branch 2 times, most recently from 3adb351 to ab31702 Compare June 4, 2016 21:01
///
/// This useful in generating class attribute as follows:
///
/// ```norun
Copy link
Owner

Choose a reason for hiding this comment

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

Why mark this norun?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied the doc comments from another macro and reworked it. I wasn't sure what norun does. I can remove it if you like.

Copy link
Owner

Choose a reason for hiding this comment

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

That's because I've been lazy 😴. I just pushed a commit that removes the norun directives (so you can see how to do this). (...I also ended up having to fix many of the examples which is why norun should be avoided...).

@dashed
Copy link
Contributor Author

dashed commented Jun 4, 2016

I refactored and I reworked the docs a bit; hope it's less awkward :)

@dashed
Copy link
Contributor Author

dashed commented Jun 4, 2016

Removing norun breaks travis.

@Stebalien
Copy link
Owner

See the updated docs (where I've removed norun). Basically, you have to wrap your example:

# #[macro_use]
# extern crate horrorshow;
# fn main() {
call_macro! {
}
# }

The # prefix means "don't display this line".

@Stebalien
Copy link
Owner

The new docs are great! Much better than I would have written, I hate writing docs... I'll merge after you fix the travis failure.

@dashed
Copy link
Contributor Author

dashed commented Jun 4, 2016

I fixed the doctests and squashed the commits.


I think we ironed out all the issues for this PR.
@Stebalien , thanks for the assistance in this PR! 👍

@Stebalien Stebalien merged commit 092c6f0 into Stebalien:master Jun 5, 2016
@Stebalien
Copy link
Owner

Thank you! It was a pleasure working with you.

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.

None yet

2 participants