leptos-rs / book

The home for the Leptos book, which can be found deployed at https://book.leptos.dev
MIT License
80 stars 60 forks source link

docs: :memo: use `nightly` Rust shorthand in the basics #51

Closed francois-blanchard closed 8 months ago

francois-blanchard commented 8 months ago

⚡ Changes

⚠️ Notes

In that case, you used both syntaxes. Since you mentioned at the beginning of the book that nightly Rust will be used in the following code examples, I kept the shorthand syntax.

gbj commented 8 months ago

No, I'd actually like to keep it as is. The distinction between these two/possibility of shortening is discussed at length just below. Your change would update the initial example but not any of the discussion, so the page is now out of sync with itself. The changed version is also more verbose than it needs to be, and clippy will warn that it's a redundant closure, which is true: {move || count()} can simply be {count}.

francois-blanchard commented 8 months ago

Thanks for your answer @gbj 👍

I think you didn't get my point. The code example doesn't work when you use the Rust stable version.

image

Line 31 set_count(3) it's the short version of set_count.set(3), that works only if you use nightly Rust.

I just wanted to make this example coherent. Does it make more sense, or do I miss something?

gbj commented 8 months ago

I understand that it doesn't work when you use the stable version.

My point is that this page uses count.get() 5 times, and the PR only changes one of them. Changing the text to only update it in one place makes it confusing when reading the whole chapter.

It would be possible to change all five of them to count(). In fact, rereading the whole chapter more closely, it appears to me that this may have been the way I wrote it originally: for example, the idea that move || count.get() is the same as count isn't exactly try, while move || count() would be. So, if you'd like to update the PR to change all the occurrences of count.get() to count() so that it's consistent, I'd be happy to re-read the new version.

francois-blanchard commented 8 months ago

It makes sense 👍.

I made the changes and I also moved 2 code comments. If you think it's not relevant, please let me know. I let you have a look.