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

Cleanup usage of unwrap() in re_dev_tools #6337

Merged
merged 4 commits into from
May 17, 2024

Conversation

Artxiom
Copy link
Contributor

@Artxiom Artxiom commented May 15, 2024

What

Part of #6330
Removes or explicitly allows unwrap() where it makes sense.

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG
  • If applicable, add a new check to the release checklist!

To run all checks from main, comment on the PR with @rerun-bot full-check.

@Wumpf Wumpf self-requested a review May 16, 2024 14:12
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.

lgtm. A bit inconsistent on the "oh this is a build tool so whatever" versus fixing them/opting out fine grained, but don't care enough 🤷

crates/re_dev_tools/src/build_search_index/meili.rs Outdated Show resolved Hide resolved
@Wumpf Wumpf added 🚜 refactor Change the code, not the functionality exclude from changelog PRs with this won't show up in CHANGELOG.md labels May 16, 2024
@Artxiom
Copy link
Contributor Author

Artxiom commented May 16, 2024

lgtm. A bit inconsistent on the "oh this is a build tool so whatever" versus fixing them/opting out fine grained, but don't care enough 🤷

👍 Thanks
It's not "..so whatever" 😄 I really check and think at least twice about every usage of unwrap(). Sometimes it just doesn't seem to make sense to remove it. When it does I remove it also in build code.
My main idea is that if something panics in production code - that's really bad, but during development, when running builds or tests, it's rarely critical and sometimes useful or even necessary.
So I try to be very conscious and consistent when differentiating between development vs. production code.
If you have any other policy regarding that, or see a place where it really shouldn't be there, please let me know.

@Wumpf
Copy link
Member

Wumpf commented May 16, 2024

Touché :). But if you argue like that I do have to point out that it's fairly inconsistent whether unwrap in build tooling is ignored blanket for an entire file - which implies ignoring all future unwraps that might be added - versus doing selectively so which prevents carelessly adding new unwraps in the future.

Concrete in crates/re_dev_tools/src/build_search_index/meili.rs you opted to ignore the single occurrence of unwrap in that module whereas in crates/re_dev_tools/src/build_search_index/ingest/rust.rs you ignore it for the entire file which means that if I later add new unwraps in there I won't notice.

But this didn't seem important enough in the context of build tooling (I very much agree with the distinction to production code here!) to make too much of a fuzz about it, that's what my lax "whatever" was about.

@Artxiom
Copy link
Contributor Author

Artxiom commented May 16, 2024

Ah yeah, I totally agree in that regard actually - I guess it's consistent and inconsistent simultaneously 😄
For these "lower priority" unwraps I decide more on a case by case basis and just make the most practical choice - meili.rs had almost no unwrap()s and rust.rs is full of unwrap()'s for paths and dom access and it would just look ugly and messy to explicitly allow unwrap everywhere.
But I really don't have an answer or opinion what's the best option for these 🤔

@Wumpf
Copy link
Member

Wumpf commented May 16, 2024

that build wheels failure there makes me nervous, all while trying to get the 0.16.0 release out. But it succeeds on main, so merging in and trying again just be sure....

@Wumpf
Copy link
Member

Wumpf commented May 16, 2024

that didn't help. will investigate tomorrow likely. Likely just something on the setup of the contributor ci 🤷

@Artxiom
Copy link
Contributor Author

Artxiom commented May 16, 2024

Good luck! I guess I can't help somehow, with the limited access I have?

@Wumpf
Copy link
Member

Wumpf commented May 17, 2024

Won't be a matter of access - there's no access restricted code dragged in into this repo at all. But likely a bit intricate and related to both ci setup (why is only contributor ci affected) and build setup (why is python build always dependent on thoswe wasm files now - recent? should bisect)

@Wumpf
Copy link
Member

Wumpf commented May 17, 2024

Issue is clearly unrelated to this PR though, so merging this in.

@Wumpf Wumpf merged commit 3ecef5a into rerun-io:main May 17, 2024
30 of 31 checks passed
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.

None yet

2 participants