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

Remove extra braces in html_nested macro #2169

Conversation

Madoshakalaka
Copy link
Contributor

Description

Refactored the html_nested macro to avoid extra braces

Fixes #2157

Checklist

  • I have run cargo make pr-flow
  • I have reviewed my own code
  • I have added tests

@Madoshakalaka
Copy link
Contributor Author

@hamza1311 sorry for the repeated effort in #2167 , I initially marked the issue #2157 as [x] would like to fix but don't know where to start. But then thought I'd give it a dab...ended up spending a whole afternoon on this. Sorry 🙏

Comment on lines +5 to +6
# test target can be optionally specified like `cargo make test html_macro`,
args = ["test", "${@}"]
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 find this useful in development

@@ -120,8 +120,8 @@ impl ToTokens for HtmlComponent {

tokens.extend(quote_spanned! {ty.span()=>
{
#[allow(clippy::unit_arg)]
Copy link
Contributor Author

@Madoshakalaka Madoshakalaka Nov 21, 2021

Choose a reason for hiding this comment

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

Previously the now __yew_props was inlined in the new call. When there are no properties (e.g. html_nested!{<MyComponent/>}), #build_props would return MyComponent::Properties::builder().build() which is the unit type, triggering the unit arg warning.
Extracting a variable solves that.

voidpumpkin
voidpumpkin previously approved these changes Nov 21, 2021
@ranile
Copy link
Member

ranile commented Nov 21, 2021

Can you also remove the allows from html! so bugs like this show up in both macros

@voidpumpkin voidpumpkin added A-yew-macro Area: The yew-macro crate macro Issues relating to our procedural or declarative macros labels Nov 21, 2021
Comment on lines +51 to +61
// can test "unused braces" warning inside the macro
// /~https://github.com/yewstack/yew/issues/2157
fn make_my_component()-> ::yew::virtual_dom::VChild<MyComponent>{
::yew::html_nested!{<MyComponent/>}
}

// can test "unused braces" warning inside the macro
// /~https://github.com/yewstack/yew/issues/2157
fn make_my_component_html()-> ::yew::Html{
::yew::html!{<MyComponent/>}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

test for both html and html_nested

@@ -168,7 +168,7 @@ impl ToTokens for HtmlRootVNode {
fn to_tokens(&self, tokens: &mut TokenStream) {
let new_tokens = self.0.to_token_stream();
tokens.extend(quote! {{
#[allow(clippy::useless_conversion, unused_braces)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this part is for the html! macro, it's no longer needed.

@@ -346,7 +346,6 @@ impl ToTokens for HtmlElement {
}
}
TagName::Expr(name) => {
#[allow(unused_braces)]
Copy link
Contributor Author

@Madoshakalaka Madoshakalaka Nov 21, 2021

Choose a reason for hiding this comment

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

Small correction in the html! marco. This is most likely misplaced. It does nothing here. No wonder, cuz the previous paragraphs repeated this pattern, this must have been copy-pasted here.

Comment on lines +364 to 370
#[allow(unused_braces)]
// e.g. html!{<@{"div"}/>} will set `#expr` to `{"div"}`
// (note the extra braces). Hence the need for the `allow`.
// Anyways to remove the braces?
let mut #vtag_name = ::std::convert::Into::<
::std::borrow::Cow::<'static, ::std::primitive::str>
>::into(#expr);
Copy link
Contributor Author

@Madoshakalaka Madoshakalaka Nov 21, 2021

Choose a reason for hiding this comment

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

This is where the misplaced #[allow] really belongs. Maybe the allow here is also liftable? I'm however too inexperienced with proc macro to figure it out. So I just left some comments instead

@Madoshakalaka
Copy link
Contributor Author

Can you also remove the allows from html! so bugs like this show up in both macros

@hamza1311 done now :D

Copy link
Member

@voidpumpkin voidpumpkin left a comment

Choose a reason for hiding this comment

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

Seems fine to me

Copy link
Contributor

@mc1098 mc1098 left a comment

Choose a reason for hiding this comment

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

Seems more than fine to me 😏

@voidpumpkin voidpumpkin merged commit 4c3d693 into yewstack:master Nov 21, 2021
@Madoshakalaka Madoshakalaka deleted the html-nested-remove-extra-brackets-warning branch November 22, 2021 03:10
Madoshakalaka added a commit to Madoshakalaka/yew that referenced this pull request Nov 22, 2021
* removed unused braces from html_nested marco

* allow specifying test name

* also fix for html! macro

* also fix for html! macro

* remove misplaced #[allow(unused_braces)]

(cherry picked from commit 4c3d693)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-yew-macro Area: The yew-macro crate macro Issues relating to our procedural or declarative macros
Projects
None yet
Development

Successfully merging this pull request may close these issues.

html_nested! inserts extra pair of brackets
4 participants