Skip to content

fix: unexpected panic in PusTmReader constructor#12

Merged
robamu merged 1 commit intous-irs:mainfrom
koloiyolo:fix/unwrap_in_pus_tm_reader_constructor
May 4, 2026
Merged

fix: unexpected panic in PusTmReader constructor#12
robamu merged 1 commit intous-irs:mainfrom
koloiyolo:fix/unwrap_in_pus_tm_reader_constructor

Conversation

@koloiyolo
Copy link
Copy Markdown
Contributor

Fixes unexpected panic in PusTmReader constructor.

@robamu
Copy link
Copy Markdown
Contributor

robamu commented May 4, 2026

thanks. i should grep and check the codebase for unwraps..

@robamu
Copy link
Copy Markdown
Contributor

robamu commented May 4, 2026

the clippy error is unrelated

@robamu robamu merged commit bc65368 into us-irs:main May 4, 2026
10 of 11 checks passed
@koloiyolo
Copy link
Copy Markdown
Contributor Author

koloiyolo commented May 5, 2026

As additional check You could introuduce new clippy rules like:

allow-unwrap-in-tests = true

disallowed-methods = [
    "std::option::Option::unwrap",
    "std::result::Result::unwrap",
]

I can do this if You'd like 😉

@koloiyolo koloiyolo deleted the fix/unwrap_in_pus_tm_reader_constructor branch May 5, 2026 07:19
@robamu
Copy link
Copy Markdown
Contributor

robamu commented May 5, 2026

Is there a lint that forces me to comment why an unwrap is okay? because i do not think unwraps are inherently bad. there are cases where I check a pre-condition which would make a function call fail, and then I just unwrap that function. I think they should probably be treated like unsafe code.

@koloiyolo
Copy link
Copy Markdown
Contributor Author

let something = result.expect("Explain why this shouldn't fail.") ;

Is good enough replacement for unwrap with comment in my opinion if this is what You are looking for.

@robamu
Copy link
Copy Markdown
Contributor

robamu commented May 5, 2026

I suppose you are right. https://doc.rust-lang.org/book/ch09-03-to-panic-or-not-to-panic.html#when-you-have-more-information-than-the-compiler mentions that would be a better approach as well.

I might look into this soon. Maybe AI can help me a bit, also curious how much it would hallucinate. any help is appreciated as well :)

there are a lot of unwraps. most of what i saw logically impossible. but still a bit of work to change all of those to expect

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants