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.

Sign up for GitHub

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

Jump to bottom

chore: Correct "builder methods" in docs and add must_use on widgets setters #655

Merged
merged 1 commit into from
Dec 6, 2023
Merged

chore: Correct "builder methods" in docs and add must_use on widgets setters #655

merged 1 commit into from
Dec 6, 2023

Conversation

TieWay59
Copy link
Contributor

@TieWay59 TieWay59 commented Nov 30, 2023 •
edited
Loading

This PR corrects the "builder methods" expressing to simple setters (see #650 #655), and gives a clearer diagnostic notice on setters must_use.

#[must_use = "method moves the value of self and returns the modified value"]

Fixes #650

orhun
orhun approved these changes Dec 2, 2023
View reviewed changes
Copy link
Member

@orhun orhun left a comment

Choose a reason for hiding this comment

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

Definitely not a word that I'm used to but +1 for doing things by the book.

joshka
joshka requested changes Dec 2, 2023
View reviewed changes
src/layout.rs Outdated Show resolved Hide resolved
This was referenced Dec 4, 2023
Merged
Merged
@TieWay59 TieWay59 requested a review from Valentin271 as a code owner December 5, 2023 07:32
@TieWay59
Copy link
Contributor Author

TieWay59 commented Dec 5, 2023

@joshka I made a WIP commit, do you think this is correct? I'll follow this pattern if this sample is fine.

@joshka
Copy link
Member

joshka commented Dec 5, 2023

@joshka I made a WIP commit, do you think this is correct? I'll follow this pattern if this sample is fine.

LGTM - go for it

@joshka
Copy link
Member

joshka commented Dec 5, 2023 •
edited
Loading

Oh BTW if you want to find all the places that could be must_use like this automatically - there's a clippy lint that you can use. It probably shouldn't be enabled by default as I suspect it would probably have some noise (things we don't want to make must_use), but it might help.

cargo clippy -- -W clippy::must-use-candidate

(And I'm happy to split that task off to another PR as it's quite large)

Edit the following will get you to a place where you can examine the potential diff more easily:

cargo clippy --fix -- -W clippy::must-use-candidate
cargo make format

Edit 4: I suspect that just omitting the message might be sufficient if we do decide to do this in bulk

@codecov Codecov
Copy link

codecov bot commented Dec 5, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4424637) 90.9% compared to head (a455798) 90.9%.

Additional details and impacted files 
@@          Coverage Diff          @@
##            main    #655   +/-   ##
=====================================
  Coverage   90.9%   90.9%           
=====================================
  Files         42      42           
  Lines      12533   12533           
=====================================
  Hits       11394   11394           
  Misses      1139    1139

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@TieWay59
Copy link
Contributor Author

TieWay59 commented Dec 5, 2023

@joshka In this PR I only focus on the fluent setter methods, keeping the docs clean and straightforward. So believe it's still OK for others to review under this criteria.

I saw you are working on #638, so I ignored the table.rs file in every commit.

@TieWay59 TieWay59 changed the title fix: Change wording in docs from "builder methods" to "fluent setters" chore: Correct "builder methods" in docs and add must_use on widgets setters Dec 5, 2023
@joshka
Copy link
Member

joshka commented Dec 5, 2023

Sounds good

joshka
joshka requested changes Dec 5, 2023
View reviewed changes
Copy link
Member

@joshka joshka left a comment

Choose a reason for hiding this comment

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

LGTM generally - a couple of small things

src/widgets.rs Outdated Show resolved Hide resolved
src/widgets/sparkline.rs Outdated Show resolved Hide resolved
@TieWay59
Copy link
Contributor Author

TieWay59 commented Dec 6, 2023

@joshka Ok, I've fixed them now.

@joshka
Copy link
Member

joshka commented Dec 6, 2023

Thanks for the small fixes. Would you mind please squashing your commits locally (so that that last commit is signed and I can merge it)?

TieWay59 added a commit to TieWay59/ratatui that referenced this pull request Dec 6, 2023
@TieWay59
chore: Correct "builder methods" in docs and add must_use on widget…
5d25008 
…s setters

Fixes ratatui#650

This PR corrects the "builder methods" expressing to simple `setters`
(see ratatui#650 ratatui#655), and gives a clearer diagnostic notice on setters `must_use`.

`#[must_use = "method moves the value of self and returns the modified value"]`

Details:

    docs: Correct wording in docs from builder methods

    Add `must_use` on layout setters

    chore: add `must_use` on widgets fluent methods

        This commit ignored `table.rs` because it is included in other PRs.

    test(gauge): fix test
@TieWay59
Copy link
Contributor Author

TieWay59 commented Dec 6, 2023

Thanks for the small fixes. Would you mind please squashing your commits locally (so that that last commit is signed and I can merge it)?

@joshka Sure, I've squashed it.

@joshka
Copy link
Member

joshka commented Dec 6, 2023

Hrm - the commit is still not gpg signed (so I get a merging is blocked error)

TieWay59 added a commit to TieWay59/ratatui that referenced this pull request Dec 6, 2023
@TieWay59
chore: Correct "builder methods" in docs and add must_use on widget…
abc0b1d 
…s setters

Fixes ratatui#650

This PR corrects the "builder methods" expressing to simple `setters`
(see ratatui#650 ratatui#655), and gives a clearer diagnostic notice on setters `must_use`.

`#[must_use = "method moves the value of self and returns the modified value"]`

Details:

    docs: Correct wording in docs from builder methods

    Add `must_use` on layout setters

    chore: add `must_use` on widgets fluent methods

        This commit ignored `table.rs` because it is included in other PRs.

    test(gauge): fix test
@TieWay59
chore: Correct "builder methods" in docs and add must_use on widget…
a455798 
…s setters

Fixes ratatui#650

This PR corrects the "builder methods" expressing to simple `setters`
(see ratatui#650 ratatui#655), and gives a clearer diagnostic notice on setters `must_use`.

`#[must_use = "method moves the value of self and returns the modified value"]`

Details:

    docs: Correct wording in docs from builder methods

    Add `must_use` on layout setters

    chore: add `must_use` on widgets fluent methods

        This commit ignored `table.rs` because it is included in other PRs.

    test(gauge): fix test
@TieWay59
Copy link
Contributor Author

TieWay59 commented Dec 6, 2023

Hrm - the commit is still not gpg signed (so I get a merging is blocked error)

@joshka Sorry, I was at my workplace without my PC. It took me some time to go back and reach my own PC.

@Valentin271 Valentin271 merged commit dd22e72 into ratatui:main Dec 6, 2023
33 checks passed
@TieWay59 TieWay59 deleted the TieWay59/issue650 branch December 6, 2023 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@orhun orhun orhun approved these changes

@joshka joshka joshka approved these changes

@mindoodoo mindoodoo Awaiting requested review from mindoodoo

@sayanarijit sayanarijit Awaiting requested review from sayanarijit

@kdheepak kdheepak Awaiting requested review from kdheepak

@Valentin271 Valentin271 Awaiting requested review from Valentin271

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Change wording in docs from "builder methods" to "fluent setters"
4 participants
@TieWay59 @joshka @orhun @Valentin271