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

Fix clippy::let_unit_value lint in propless components #2970

Merged
merged 1 commit into from Dec 8, 2022

Conversation

WorldSEnder
Copy link
Member

Description

Fixes #2931

Checklist

  • I have reviewed my own code
  • I have added tests.
    I can't figure out how to run clippy lints as part of the try_build macro tests. Any tip is appreciated. Otherwise, we'd have to setup another test rig just for clippy.

@github-actions
Copy link

Visit the preview URL for this PR (updated for commit 0c6238b):

https://yew-rs-api--pr2970-fix-unit-let-fm2m2jbt.web.app

(expires Sat, 26 Nov 2022 12:46:51 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@github-actions
Copy link

Benchmark - SSR

Yew Master

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 366.277 402.635 383.686 12.689
Hello World 10 707.992 744.709 717.554 11.068
Function Router 10 2376.719 2518.400 2426.456 41.827
Concurrent Task 10 1009.049 1010.577 1009.774 0.532

Pull Request

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 373.765 396.599 382.267 7.899
Hello World 10 690.043 747.658 712.212 15.654
Function Router 10 2459.871 2536.938 2505.135 25.144
Concurrent Task 10 1008.444 1010.496 1009.505 0.569

@github-actions
Copy link

Size Comparison

examples master (KB) pull request (KB) diff (KB) diff (%)
async_clock 108.684 108.683 -0.001 -0.001%
boids 172.901 172.902 +0.001 +0.001%
communication_child_to_parent 92.557 92.557 0 0.000%
communication_grandchild_with_grandparent 107.296 107.289 -0.007 -0.006%
communication_grandparent_to_grandchild 103.175 103.173 -0.002 -0.002%
communication_parent_to_child 89.663 89.663 0 0.000%
contexts 109.899 109.896 -0.004 -0.004%
counter 87.582 87.581 -0.001 -0.001%
counter_functional 88.059 88.059 0 0.000%
dyn_create_destroy_apps 90.435 90.435 0 0.000%
file_upload 102.149 102.148 -0.001 -0.001%
function_memory_game 165.372 165.372 0 0.000%
function_router 349.622 349.605 -0.017 -0.005%
function_todomvc 160.175 160.166 -0.009 -0.005%
futures 224.237 224.236 -0.001 -0.000%
game_of_life 107.107 107.107 0 0.000%
immutable 184.840 184.840 0 0.000%
inner_html 84.049 84.047 -0.002 -0.002%
js_callback 113.461 113.451 -0.010 -0.009%
keyed_list 198.071 198.069 -0.002 -0.001%
mount_point 87.191 87.193 +0.002 +0.002%
nested_list 114.186 114.188 +0.003 +0.003%
node_refs 95.057 95.057 0 0.000%
password_strength 1549.361 1549.359 -0.002 -0.000%
portals 98.521 98.522 +0.002 +0.002%
router 319.404 319.414 +0.010 +0.003%
simple_ssr 154.118 154.115 -0.003 -0.002%
ssr_router 394.606 394.601 -0.006 -0.001%
suspense 111.212 111.213 +0.001 +0.001%
timer 90.472 90.470 -0.002 -0.002%
todomvc 141.471 141.470 -0.001 -0.001%
two_apps 88.242 88.238 -0.004 -0.004%
web_worker_fib 154.486 154.478 -0.009 -0.006%
webgl 86.766 86.768 +0.002 +0.002%

✅ None of the examples has changed their size significantly.

@WorldSEnder
Copy link
Member Author

@Starwort I think this should be enough to fix the clippy lints. We don't have an automated test for these lints setup, so do you mind manually testing it. At least with today's 1.67 nightly clippy, the reproduction from the bug report doesn't show any warnings for me.

@Starwort
Copy link

so do you mind manually testing it

I tried replacing, in Cargo.toml, the dependency yew = "0.19.3" with the dependency yew = { git = "https://github.com/WorldSEnder/yew", branch="fix-unit-let" }, but now I'm getting a load of compile errors including in totally unrelated places, so I can't test it in situ (that's what you were looking for, yes?)

image
1155 errors in openssl alone!

If you can tell me steps to repair the build, I'd be happy to post updated results; if you'd like to just run it on my codebase, the project in question is https://github.com/Starwort/gear-miner/ (the lint is disabled globally in src/app.rs) (and yes I'm aware it's full of nasty hacks)

@WorldSEnder
Copy link
Member Author

It's taking a bit longer, because the app would have to be converted to yew's git version. Should not require any changes to component logic, but a few extra #[hook] annotations here and there. I am not getting errors from openssl and I can reproduce the original bug report with the repo, at least.

@hamza1311
Copy link
Member

The openssl errors are unrelated to this change. This PR looks good so I'm going to go ahead and merge it

@hamza1311 hamza1311 merged commit 9e980a3 into yewstack:master Dec 8, 2022
@hamza1311 hamza1311 added the A-yew-macro Area: The yew-macro crate label Dec 8, 2022
@WorldSEnder WorldSEnder deleted the fix-unit-let branch January 6, 2023 20:46
WorldSEnder added a commit to WorldSEnder/yew that referenced this pull request Jan 10, 2023
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

let_unit_value violation within html macro
3 participants