Skip to content

fix: pre_hook (sync) make request mutable - #825

Merged
ahl merged 3 commits into
oxidecomputer:mainfrom
drahnr:bernhard-chore-mic-mut-hook
Sep 9, 2026
Merged

fix: pre_hook (sync) make request mutable#825
ahl merged 3 commits into
oxidecomputer:mainfrom
drahnr:bernhard-chore-mic-mut-hook

Conversation

@drahnr

@drahnr drahnr commented Jun 8, 2024

Copy link
Copy Markdown
Contributor

Obsoletes #823

ahl
ahl reviewed Jun 12, 2024
View reviewed changes

@ahl ahl left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm sort of surprised that we don't have a test that validates this. What will existing users of pre_hook experience other than a type error if they've specified the type in their pre_hook closure or fn?

@drahnr

drahnr commented Jun 13, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I'm sort of surprised that we don't have a test that validates this. What will existing users of pre_hook experience other than a type error if they've specified the type in their pre_hook closure or fn?

Can you rephrase, I am not sure I understand what your concern is. Do you want an interface check for the provided TokenStream or are you concern re change in behviour for existing users that expect an &Request as argument?

@ahl

ahl commented Jun 13, 2024

Copy link
Copy Markdown
Collaborator

If there is an existing user of the pre_hook interface, what will those users experience with this change? Will everything just work or will compilation fail with an error?

@drahnr

drahnr commented Jan 1, 2026

Copy link
Copy Markdown
Contributor Author

Happy new year!

Mildly belated response; the user won't see a delta, since a we can call any fn foo(&T) with foo(&mut t).

ahl and others added 2 commits September 9, 2026 12:56
@ahl @claude
Merge branch 'main' into bernhard-chore-mic-mut-hook
bae49be 
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FsEsN8H4VAfy3enDy5kiGU
@ahl @claude
add compile test for pre_hook signatures; update macro doc for &mut
b5dc9fc 
The sync pre_hook now receives &mut reqwest::Request. Validate at compile
time both that a hook can mutate the request and that a hook written
against &reqwest::Request still compiles via &mut -> & coercion.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FsEsN8H4VAfy3enDy5kiGU
Copilot AI lite review requested due to automatic review settings September 9, 2026 19:56
Copilot AI reviewed Sep 9, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review is ineligible. To be eligible to request a review, you need a paid Copilot license, or your organization must enable Copilot code review.

ahl
ahl approved these changes Sep 9, 2026
View reviewed changes

@ahl ahl left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

thanks for this; sorry for the very long delay

@ahl
ahl merged commit aa55fe1 into oxidecomputer:main Sep 9, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort, defaults to Lite
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@ahl ahl ahl 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.

3 participants

@drahnr @ahl