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

url: Allow overriding the error-handling #799

Merged
merged 4 commits into from
Dec 18, 2023
Merged

Conversation

robinlinden
Copy link
Owner

This enables us to e.g. display parse-errors in the UI and other things
that aren't just printing all errors to the console.

The default behaviour is still what it was before this change, but after this change, we could probably drop the spdlog-dependency of //url and instead set up spdlog-logging in the tests and the browser instead if we want.

@robinlinden robinlinden marked this pull request as ready for review December 17, 2023 16:41
Copy link
Collaborator

@Zer0-One Zer0-One left a comment

Choose a reason for hiding this comment

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

Wanted to replace that with a callback at some point, but I forgot <_<; the logging was really just there for debugging while I was working on it.

If you wanna drop the spdlog dependency now instead of later, feel free.

This enables us to e.g. display parse-errors in the UI and other things
that aren't just printing all errors to the console.
@robinlinden robinlinden force-pushed the url-allow-error-access branch 3 times, most recently from 8b4aff0 to f82564f Compare December 17, 2023 22:05
@robinlinden robinlinden force-pushed the url-allow-error-access branch from f82564f to 82bb14a Compare December 17, 2023 22:26
@robinlinden
Copy link
Owner Author

Wanted to replace that with a callback at some point, but I forgot <_<; the logging was really just there for debugging while I was working on it.

Heh, I had a feeling that was the case when one of the spdlog prints was something like spdlog::info("IPv6 parse failure 2"). :P I think keeping all the descriptions and making it possible to use the logging as a consumer of //url is neat and useful, but let's not default to it then.

If you wanna drop the spdlog dependency now instead of later, feel free.

I had a look at dropping the spdlog-dependency of //url. PTAL; the codecov patch failure is because we can't test uts46-creation failure, which I think should be impossible (outside of oom) now that we link the data statically.

@robinlinden robinlinden merged commit 82bb14a into master Dec 18, 2023
22 of 23 checks passed
@robinlinden robinlinden deleted the url-allow-error-access branch December 18, 2023 08:36
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