ContextQMD
Back to Codeberg

Tracking remaining unwrap and expect usage #534

forgejo-contrib-forgejo-cli-issues-534.md

latest10.1 KB
Original Source

forgejo-contrib/forgejo-cli

Watch20

Star402

Fork

You've already forked forgejo-cli

51

CodeIssues 44Pull requests 8Releases 11Packages 2WikiActivity

Tracking remaining unwrap and expect usage #534

New issue

Open

opened 2026-06-28 10:18:03 +02:00 by Fluffinity · 2 comments

Fluffinity commented 2026-06-28 10:18:03 +02:00

Contributor

Copy link

Issue

Right now we have 44 uses of these functions in the codebase. All of them are potential grounds for crashes. This issue serves as a place for discussing every such usage and whether it should be replaced with a report. In other words: which things can simply fail and which are actually assertions? The current uses are in:

  • issues/template/yaml.rs:226
  • completion.rs:55
  • issues.rs:478
  • keys.rs:92
  • localization.rs:16-20,25,176,178-181,302,318,357,385,435
  • main.rs:120,313,895,901,960
  • prs.rs:1118,1125,1486,14901553 (when grep'ing you see two more, but they are commented out)
  • release.rs:255,438,459
  • repo.rs:188,190,644,683
  • auth.rs:173,220,223,251,319,324,328

The unwrap in auth.rs:220 has already caused a reported crash, so I'll prepare a PR to convert this to an eyre report.

Code of Conduct

  • I agree to act in accordance with the CoC & AI Agreement.
  • This issue was not generated by an LLM, even in part.

Issue Right now we have 44 uses of these functions in the codebase. All of them are potential grounds for crashes. This issue serves as a place for discussing every such usage and whether it should be replaced with a report. In other words: which things can simply fail and which are actually assertions? The current uses are in: - issues/template/yaml.rs:226 - completion.rs:55 - issues.rs:478 - keys.rs:92 - localization.rs:16-20,25,176,178-181,302,318,357,385,435 - main.rs:120,313,895,901,960 - prs.rs:1118,1125,1486,14901553 (when grep'ing you see two more, but they are commented out) - release.rs:255,438,459 - repo.rs:188,190,644,683 - auth.rs:173,220,223,251,319,324,328 The unwrap in auth.rs:220 has already caused a reported crash, so I'll prepare a PR to convert this to an eyre report. ### Code of Conduct - [x] I agree to act in accordance with the CoC & AI Agreement. - [x] This issue was not generated by an LLM, even in part.

Fluffinity referenced this issue 2026-06-28 10:45:47 +02:00 fix(auth): handle detached opening error gracefully #535

Cyborus commented 2026-06-28 16:06:03 +02:00

Member

Copy link

I consider it a non-goal to clear the project of .unwraps, .expects, or panic!s. My general rule is a panic should happen if there's a programming bug in forgejo-cli, and an ordinary error should happen for anything else (i.e. bad user input, issue with environment, etc). I'll include this in CONTRIBUTING.md.

By that rule, removing the unwrap at auth.rs:220 is correct; the user shouldn't encounter a panic, their environment not having a browser shouldn't cause one.

I consider it a non-goal to clear the project of .unwraps, .expects, or panic!s. My general rule is a panic should happen if there's a programming bug in forgejo-cli, and an ordinary error should happen for anything else (i.e. bad user input, issue with environment, etc). I'll include this in CONTRIBUTING.md. By that rule, removing the unwrap at auth.rs:220 is correct; the user shouldn't encounter a panic, their environment not having a browser shouldn't cause one.

Fluffinity commented 2026-06-28 16:47:15 +02:00

Author

Contributor

Copy link

I consider it a non-goal to clear the project of .unwraps, .expects, or panic!s.

I agree. Although I would say, that every time we assert something it should be done via expect. This allows us to at least state what we even assert. unwrap doesn't give us that info. So all the current unwraps should at least be converted into expect

> I consider it a non-goal to clear the project of .unwraps, .expects, or panic!s. I agree. Although I would say, that every time we assert something it should be done via expect. This allows us to at least state what we even assert. unwrap doesn't give us that info. So all the current unwraps should at least be converted into expect

👍 1

Sign in to join this conversation.

No Branch/Tag specified

BranchesTags

main

renovate/lock-file-maintenance

renovate/serde-saphyr-0.x

docs/contributing

no-port-in-refspec

login/v16.next.forgejo.org

crates-io-publish

0.5.x

compile-time-fluent

0.4.x

localization-alias-demo

api_url_field

v0.5.0

v0.4.1

v0.4.0

v0.3.0

v0.2.0

v0.1.1

v0.1.0

v0.0.4

v0.0.3

v0.0.2

v0.0.1

Labels

Clear labels[ Kind/Breaking Breaking change that won't be backward compatible

](#)[ Kind/Bug Something is not working

](#)[ Kind/Design Discussion about UI/UX design

](#)[ Kind/Documentation Documentation changes

](#)[ Kind/Enhancement Improve existing functionality

](#)[ Kind/Feature New functionality

](#)[ Kind/Security This is security issue

](#)[ Kind/Testing Issue or pull request related to testing

](#)[ Kind/Upstream This is an issue with upstream software (Forgejo) that is probably not our fault

](#)

[ Priority

Critical The priority is critical

](#)[ Priority

High The priority is high

](#)[ Priority

Low The priority is low

](#)[ Priority

Medium The priority is medium

](#)

[ Reviewed

Confirmed Issue has been confirmed

](#)[ Reviewed

Duplicate This issue or pull request already exists

](#)[ Reviewed

Invalid Invalid issue

](#)[ Reviewed

Won't Fix This issue won't be fixed

](#)

[ Status

Abandoned Somebody has started to work on this but abandoned work

](#)[ Status

Blocked Something is blocking this issue or pull request

](#)[ Status

Need More Info Feedback is required to reproduce issue or to continue work

](#)

[ Suspicious

](#)

No labels Kind/Breaking Kind/Bug Kind/Design Kind/Documentation Kind/Enhancement Kind/Feature Kind/Security Kind/Testing Kind/Upstream [ Priority

Critical ](/forgejo-contrib/forgejo-cli/issues?labels=173012) [ Priority

High ](/forgejo-contrib/forgejo-cli/issues?labels=173013) [ Priority

Low ](/forgejo-contrib/forgejo-cli/issues?labels=173015) [ Priority

Medium ](/forgejo-contrib/forgejo-cli/issues?labels=173014) [ Reviewed

Confirmed ](/forgejo-contrib/forgejo-cli/issues?labels=173007) [ Reviewed

Duplicate ](/forgejo-contrib/forgejo-cli/issues?labels=173005) [ Reviewed

Invalid ](/forgejo-contrib/forgejo-cli/issues?labels=173006) [ Reviewed

Won't Fix ](/forgejo-contrib/forgejo-cli/issues?labels=173008) [ Status

Abandoned ](/forgejo-contrib/forgejo-cli/issues?labels=173011) [ Status

Blocked ](/forgejo-contrib/forgejo-cli/issues?labels=173010) [ Status

Need More Info ](/forgejo-contrib/forgejo-cli/issues?labels=173009) Suspicious

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

forgejo-contrib/forgejo-cli#534

WritePreview

Loading…

Add table

| Rows | | | Columns | |

CancelOK

Add a link

Url Description Hint: With a URL in your clipboard, you can paste directly into the editor to create a link.

CancelOK

CancelSave

Reference in a new issue

Repository

forgejo-contrib/forgejo-cli

Title

Body

Create issue

No description provided.

Delete branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?

No Yes