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

Split up error hooks to be more modular, making ordering explicit #1086

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

sirosen
Copy link
Member

@sirosen sirosen commented Mar 5, 2025

Two commits:

  • First a "boring" preparatory move to move some strings and messaging helpers to a distinct module

  • Second, "the change" itself: "Refactor to make error handler ordering explicit"

    Rather than making order driven by definition order, expose the declared hook type (was _RegisteredHook), and convert the decorated functions into the declared hook objects. Then, ordering can be done by any sorting function -- currently, a strict ordered listing with no implicit ordering semantics -- and we can explicitly register the results.

    This allows us to break the giant hook module down into its constituent parts which can be further refined and reorganized over time.

sirosen added 2 commits March 5, 2025 11:44
Move all of the message rendering and fixed strings into a dedicated
`messages` module, and take a pass over the resulting hooks module to
try to make things more compact and readable.
Rather than making order driven by definition order, expose the
declared hook type (was `_RegisteredHook`), and convert the decorated
functions into the declared hook objects. Then, ordering can be done
by any sorting function -- currently, a strict ordered listing with no
implicit ordering semantics -- and we can explicitly register the
results.

This allows us to break the giant hook module down into its
constituent parts which can be further refined and reorganized over
time.
@sirosen sirosen added the no-news-is-good-news This change does not require a news file label Mar 5, 2025
If the 'message' field is missing (as observed in real cases), the
hook fails hard at the start.

To rectify, rewrite to rescope where and how we dig into
$.error.message as the error message. Do that lookup under a KeyError
handler, and if that fails, failover to `$.error.detail` even if it is
of length 1.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-news-is-good-news This change does not require a news file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant