Skip to content

Adds HEAD request functionality with http_head #21

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

Merged
merged 4 commits into from
Jul 28, 2025

Conversation

thomascjohnson
Copy link
Contributor

@thomascjohnson thomascjohnson commented May 21, 2025

Added as a result of this issue: #20

This needs some cleanup due to the difference in response values for HEAD compared to GET and POST.

@thomascjohnson thomascjohnson changed the title Adds an initial implementation of http_head Adds HEAD request functionality with http_head May 21, 2025
@thomascjohnson
Copy link
Contributor Author

Just doing some testing on my end and my use case is to do up to hundreds of HEAD requests at a time. I'm seeing segmentation faults in some cases, and in general it would be good to have a progress bar on this. This is outside of the scope of this change, but I'm curious what your thoughts are on that.

@lmangani
Copy link
Collaborator

lmangani commented May 21, 2025

@thomascjohnson we'll familiarize with the change and run some tests to form suggestions/feedback
@ahuarte47 do you have any comments or preferences when it come to this feature?

@thomascjohnson
Copy link
Contributor Author

To follow up on the seg faults, it's caused by URLs that can't connect and the headers being processed when they aren't available in the result. Just pushed a fix for that. I also fixed the issue with decomposition declarations.

@lmangani
Copy link
Collaborator

lmangani commented May 21, 2025

FYI The build failures do not appear to be the code's fault - I'll restart the jobs to confirm.

EDIT: tests passing

@thomascjohnson
Copy link
Contributor Author

Thanks @lmangani! Looks like some of those URLs can be flaky? Any thoughts on adding tests with the extension httpserver?

@lmangani
Copy link
Collaborator

Thanks @lmangani! Looks like some of those URLs can be flaky? Any thoughts on adding tests with the extension httpserver?

yes indeed. We need a better public service for testing.

@thomascjohnson
Copy link
Contributor Author

thomascjohnson commented Jul 28, 2025

Thanks Lorenzo, I was actually trying to setup something for testing yesterday but it's challenging with the DuckDB testing framework.

@lmangani
Copy link
Collaborator

@thomascjohnson i'm redesigning the test but i didn't wanna hold your PR back, unless you still have changes to add. We can re-add the tests all together once its merged and pick a more reliable target, although issues are always possible in actions.

@lmangani lmangani merged commit 5d13239 into quackscience:main Jul 28, 2025
33 of 66 checks passed
@lmangani
Copy link
Collaborator

Merged! Thanks for your contribution @thomascjohnson 💯 I'll go ahead and rebuild the tests on a separate PR.

@lmangani
Copy link
Collaborator

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