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::unwrap_used in re_viewer with Option::expect #8745

Merged

Conversation

brody4hire
Copy link
Contributor

@brody4hire brody4hire commented Jan 20, 2025

Related

Part of: #6330

What

  • remove allow directive & TODO comment from crates/viewer/re_viewer/src/lib.rs
  • use . unwrap_or_else with panic in 1 case
  • use focused #[allow(clippy::unwrap_used)] directives in limited, focused cases
  • use .expect with descriptive messages in all other cases

(no changelog updates)

General rationale:

Most of the cases using .unwrap seem to need these to work. I think the best solution is to use .expect with a clear message in most of these cases.

Sanity checks done:

These succeed for me with no warnings:

  • cargo clippy -p re_viewer --all-features
  • cargo build -p re_viewer --all-features
  • cargo fmt does not introduce inconsistent formatting of the updated code

@Wumpf Wumpf self-requested a review January 22, 2025 09:15
@Wumpf Wumpf added 🚜 refactor Change the code, not the functionality exclude from changelog PRs with this won't show up in CHANGELOG.md labels Jan 22, 2025
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

thank you!!

@brody4hire brody4hire marked this pull request as draft January 22, 2025 17:39
@brody4hire brody4hire force-pushed the resolve-clippy-unwrap_used-in-re_viewer branch from 41931d1 to e0d4ff3 Compare January 22, 2025 18:05
@brody4hire brody4hire force-pushed the resolve-clippy-unwrap_used-in-re_viewer branch from a719ec0 to fd9e54a Compare January 22, 2025 18:18
@brody4hire brody4hire marked this pull request as ready for review January 22, 2025 18:19
Copy link
Contributor Author

@brody4hire brody4hire left a comment

Choose a reason for hiding this comment

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

@Wumpf I have shamelessly thrown away your cleanup work in favor of using .expect("... (internal error)") calls (I am assuming no offense will be taken). I think this should both reduce the number of lines in most cases and clarify the code a little better.

I think this should be ready now. Please do feel free to apply any other cleanup you think may be needed & be sure to add yourself as a co-author.

@brody4hire brody4hire marked this pull request as draft January 24, 2025 15:59
@brody4hire brody4hire force-pushed the resolve-clippy-unwrap_used-in-re_viewer branch from a1df8c2 to 82a262c Compare January 24, 2025 16:11
@brody4hire brody4hire marked this pull request as ready for review January 24, 2025 16:24
@brody4hire brody4hire force-pushed the resolve-clippy-unwrap_used-in-re_viewer branch from 5e7c062 to be85177 Compare January 30, 2025 19:53
@brody4hire brody4hire changed the title resolve clippy::unwrap_used in re_viewer resolve clippy::unwrap_used in re_viewer with expect Jan 30, 2025
@brody4hire brody4hire changed the title resolve clippy::unwrap_used in re_viewer with expect fix clippy::unwrap_used in re_viewer with Option::expect Jan 30, 2025
@brody4hire brody4hire marked this pull request as draft January 30, 2025 20:00
@brody4hire brody4hire force-pushed the resolve-clippy-unwrap_used-in-re_viewer branch from cfa9dea to 37fe192 Compare January 30, 2025 20:14
@brody4hire brody4hire marked this pull request as ready for review January 30, 2025 20:15
@brody4hire
Copy link
Contributor Author

I have rebased (2x), removed " (internal error)" from the end of the expect messages, and updated an error message to keep cargo fmt from flatting an update. Please let me know if anything else is needed from me on this.

Copy link
Member

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Thank you for this!

@Wumpf Wumpf merged commit 073fcc3 into rerun-io:main Jan 31, 2025
5 checks passed
emilk added a commit that referenced this pull request Jan 31, 2025
This should produce better error messages when running Rerun
Wasm outside of a browser.

Required because of
* #8745
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exclude from changelog PRs with this won't show up in CHANGELOG.md 🚜 refactor Change the code, not the functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants