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

Don't panic when parsing invalid Url to Uri #1399

Closed
wants to merge 3 commits into from
Closed

Don't panic when parsing invalid Url to Uri #1399

wants to merge 3 commits into from

Conversation

mre
Copy link
Contributor

@mre mre commented Dec 4, 2021

This provides a way to handle the parsing error up the call stack instead of panicking.
See #668.

This change should not impact the public API as Pending still gets returned from execute_request.

@timvisee fyi

@mre
Don't panic when parsing invalid Url to Uri
1539a7e 
Instead propagate this parsing issue up to the
calling function as a Result.
See seanmonstar#668
@mre mre mentioned this pull request Dec 4, 2021
Closed
@mre mre mentioned this pull request Jan 28, 2022
Closed
@mre
Copy link
Contributor Author

mre commented Jan 31, 2022

Can one of the maintainers have a look here? This is causing some issues in a downstream project. lycheeverse/lychee#483

@mre mre mentioned this pull request Feb 7, 2022
Closed
@mre mre mentioned this pull request Feb 28, 2022
Closed
@dend
Copy link

dend commented Mar 7, 2022

@seanmonstar any chance you could take a look at this PR? This is impacting some downstream projects that are breaking because of a library-caused panic.

@dend dend mentioned this pull request Mar 17, 2022
Closed
@nipunn1313
Copy link
Contributor

Hi - I'd like to politely check in on this PR (@seanmonstar). My downstream project is also affected by this.

Trying to help out, I did a code review pass on this and it looks reasonable. A unit test that exercises a bad URI path would be a nice addition, to make sure this doesn't regress.

seanmonstar
seanmonstar approved these changes Apr 6, 2023
View reviewed changes
Copy link
Owner

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this!

@filips123 filips123 mentioned this pull request Jun 14, 2023
Closed
@filips123
Copy link

Is there anything else that needs to be done before this can be merged? There still seem to be some failing tests and the branch is out-of-date. Any chance this can be fixed and merged? I also have this problem in my project, as invalid URLs panick and abort the whole program instead of just returning the error that can be handled.

@jcfj jcfj mentioned this pull request Aug 9, 2023
Open
@LucasPickering LucasPickering mentioned this pull request Nov 15, 2023
Merged
@LucasPickering
Copy link
Contributor

I just opened a new PR to revive these changes: #2040

seanmonstar pushed a commit that referenced this pull request Jan 29, 2024
@LucasPickering @mre
fix panic when parsing invalid Url to Uri (#2040)
5d8443d 
Instead propagate this parsing issue up to the
calling function as a Result.
See #668
Original PR: #1399

Co-authored-by: Matthias <matthias-endler@gmx.net>
@seanmonstar
Copy link
Owner

Merged via #2040

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

@seanmonstar seanmonstar seanmonstar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mre @dend @nipunn1313 @filips123 @LucasPickering @seanmonstar