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

Forbid unwrap() in our code #6330

Open
6 of 19 tasks
Artxiom opened this issue May 15, 2024 · 3 comments
Open
6 of 19 tasks

Forbid unwrap() in our code #6330

Artxiom opened this issue May 15, 2024 · 3 comments
Labels
😤 annoying Something in the UI / SDK is annoying to use good first issue Good for newcomers 🚜 refactor Change the code, not the functionality
Milestone
Spring Cleaning

Comments

@Artxiom
Copy link
Contributor

Artxiom commented May 15, 2024 •
edited by Wumpf
Loading

@Wumpf
Follow up for #3408.
After the initial refactor there are still some crates left which use unwrap:

  • re_data_store
  • re_dev_tools
  • re_entity_db
  • re_format_arrow
  • re_log_types
  • re_query
  • re_renderer
  • re_renderer_examples/*
  • re_sdk
  • re_space_view_spatial
  • re_space_view_tensor
  • re_space_view_time_series
  • re_time_panel
  • re_types_builder
  • re_types_core
  • re_viewer
  • re_viewport
  • re_ws_comms
  • snippets

They are all marked with:

// TODO(#3408): remove unwrap()
#![allow(clippy::unwrap_used)]
@Artxiom Artxiom added other Generated by the "Other" issue template 👀 needs triage This issue needs to be triaged by the Rerun team labels May 15, 2024
@Wumpf Wumpf added good first issue Good for newcomers 😤 annoying Something in the UI / SDK is annoying to use 🚜 refactor Change the code, not the functionality and removed other Generated by the "Other" issue template 👀 needs triage This issue needs to be triaged by the Rerun team labels May 15, 2024
This was referenced May 15, 2024
Merged
Merged
emilk pushed a commit that referenced this issue May 16, 2024
@Artxiom
Cleanup usage of unwrap() in re_data_store (#6334)
692b6b6 
<!--
Open the PR up as a draft until you feel it is ready for a proper
review.

Do not make PR:s from your own `main` branch, as that makes it difficult
for reviewers to add their own fixes.

Add any improvements to the branch as new commits to make it easier for
reviewers to follow the progress. All commits will be squashed to a
single commit once the PR is merged into `main`.

Make sure you mention any issues that this PR closes in the description,
as well as any other related issues.

To get an auto-generated PR description you can put "copilot:summary" or
"copilot:walkthrough" anywhere.
-->

### What

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

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested the web demo (if applicable):
* Using examples from latest `main` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/6334?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/6334?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] 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](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)!

- [PR Build Summary](https://build.rerun.io/pr/6334)
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)

To run all checks from `main`, comment on the PR with `@rerun-bot
full-check`.
Wumpf added a commit that referenced this issue May 17, 2024
@Artxiom @Wumpf
Cleanup usage of unwrap() in re_dev_tools (#6337)
3ecef5a 
<!--
Open the PR up as a draft until you feel it is ready for a proper
review.

Do not make PR:s from your own `main` branch, as that makes it difficult
for reviewers to add their own fixes.

Add any improvements to the branch as new commits to make it easier for
reviewers to follow the progress. All commits will be squashed to a
single commit once the PR is merged into `main`.

Make sure you mention any issues that this PR closes in the description,
as well as any other related issues.

To get an auto-generated PR description you can put "copilot:summary" or
"copilot:walkthrough" anywhere.
-->

### What

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

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested the web demo (if applicable):
* Using examples from latest `main` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/6337?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/6337?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG
* [x] If applicable, add a new check to the [release
checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)!

- [PR Build Summary](https://build.rerun.io/pr/6337)
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)

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

---------

Co-authored-by: Andreas Reich <r_andreas2@web.de>
Co-authored-by: Andreas Reich <andreas@rerun.io>
This was referenced May 17, 2024
Merged
Closed
Wumpf pushed a commit that referenced this issue May 27, 2024
@Artxiom @emilk
Refactor usage of unwrap in re_db, re_format_arrow & re_log_types (#6380
36ad454 
)

<!--
Open the PR up as a draft until you feel it is ready for a proper
review.

Do not make PR:s from your own `main` branch, as that makes it difficult
for reviewers to add their own fixes.

Add any improvements to the branch as new commits to make it easier for
reviewers to follow the progress. All commits will be squashed to a
single commit once the PR is merged into `main`.

Make sure you mention any issues that this PR closes in the description,
as well as any other related issues.

To get an auto-generated PR description you can put "copilot:summary" or
"copilot:walkthrough" anywhere.
-->

### What

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

Affected crates: `re_entity_db`, `re_format_arrow`, `and re_log_types`

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested the web demo (if applicable):
* Using examples from latest `main` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/6380?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/6380?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG
* [x] If applicable, add a new check to the [release
checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)!

- [PR Build Summary](https://build.rerun.io/pr/6380)
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)

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

---------

Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
@Artxiom Artxiom mentioned this issue Jun 13, 2024
Merged
5 tasks
Wumpf pushed a commit that referenced this issue Jun 13, 2024
@Artxiom
Cleanup usage of unwrap() in re_query (#6564)
7734fcc 
<!--
Open the PR up as a draft until you feel it is ready for a proper
review.

Do not make PR:s from your own `main` branch, as that makes it difficult
for reviewers to add their own fixes.

Add any improvements to the branch as new commits to make it easier for
reviewers to follow the progress. All commits will be squashed to a
single commit once the PR is merged into `main`.

Make sure you mention any issues that this PR closes in the description,
as well as any other related issues.

To get an auto-generated PR description you can put "copilot:summary" or
"copilot:walkthrough" anywhere.
-->

### What

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

Affected crates: `re_query`

Note: most of the usages of unwrap in this crate were already checked so
they are explicitly allowed.

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested the web demo (if applicable):
* Using examples from latest `main` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/6564?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/6564?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG
* [x] If applicable, add a new check to the [release
checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)!

- [PR Build Summary](https://build.rerun.io/pr/6564)
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)

To run all checks from `main`, comment on the PR with `@rerun-bot
full-check`.
@Artxiom
Copy link
Contributor Author

Artxiom commented Aug 2, 2024

Hello. I want to take this issue (I started with re-renderer)

@ilyaaay If you want to continue on this one please start with the next one (I would suggest re_sdk) because I already did something for re_renderer, only didn't have time to finish it.

@Artxiom
Copy link
Contributor Author

Artxiom commented Aug 3, 2024

@ilyaaay I realized it's not a huge amount of changes yet so I pushed my branch here.

Feel free to pick it up - I will be super busy probably until beginning of next year.
Just please @mention me somewhere when you do so, so I don't pick it up accidentally again.

@riccardocinti
Copy link

Hi everyone. I'd like to contribute to this issue and I started working on the top/re_sdk and the store/re_sdk_comms crates.

@teh-cmc teh-cmc mentioned this issue Oct 19, 2024
Closed
1 task
@Wumpf Wumpf mentioned this issue Nov 6, 2024
Open
@emilk emilk changed the title Further check and refactor unwrap() usage Forbid unwrap() in our code Jan 13, 2025
@emilk emilk added this to the Spring Cleaning milestone Jan 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
😤 annoying Something in the UI / SDK is annoying to use good first issue Good for newcomers 🚜 refactor Change the code, not the functionality
Projects
None yet
Milestone
Spring Cleaning
Development

No branches or pull requests
4 participants
@emilk @Wumpf @riccardocinti @Artxiom