fix: lead APIError's string form with the error message - #724
fix: lead APIError's string form with the error message#724Max Azatian (HardMax71) wants to merge 3 commits into
Conversation
str(APIError) started with a newline and printed message: None for every generated error model, ignoring the primary_message the generator emits, so any log line or error title built from the first line was empty. The first line now carries the message (primary_message, then message, then the class name), the status code and the error code; the nested error payload follows on a second line.
There was a problem hiding this comment.
🟢 Approval recommended
The formatting change is localized, matches the stated issue/expected behavior, and is covered by targeted new tests for the key scenarios.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
Vincent Biret (baywet)
left a comment
Vincent Biret (baywet)
left a comment
There was a problem hiding this comment.
Thanks for the contribution!
The base class has no notion of an error code; `error.code` is the OData error contract as Graph exposes it. The first line is now the message and the status only, the code stays visible on the error line.
|
| error = getattr(self, "error", None) | ||
| message = getattr(self, "primary_message", None) or self.message or type(self).__name__ |
There was a problem hiding this comment.
I'm also confused about those two fields: they are not present in the base class? or defined here?
There was a problem hiding this comment.
Not on the base class, both come from the generated error models. error is the field the generator makes for the API's error schema, on Graph that is ODataError.error. The base class already read it with getattr before this PR (added in #137). primary_message is a property the generator adds to every error model whose schema marks a field with x-ms-primary-error-message (PythonRefiner.cs#L153, CommonLanguageRefiner.cs#L1577), on Graph here.
So the base class only uses them when the subclass has them. If you'd like to, I can add primary_message to APIError as a property that returns self.message, the generated classes already override it and __str__ can then read it directly.
There was a problem hiding this comment.
Thank you for the additional information.
I think the python implementation has strayed from the other languages here. The behaviour is typically for the rendering method to be overloaded and generated, see this example which is based on this class which exposes additional properties (like the status code and response headers), but doesn't rely on reflection to format the message.
Unless Python has some peculiar language constraints I can't remember, we should probably align things here.
Overview
APIError.__str__returned a multi-line string that started with a newline and printedmessage: Nonefor every generated error model, because the generator never setsmessage; it emits aprimary_messageproperty that__str__ignored. Anything that titles an exception by its first line (Sentry does exactlyvalue.splitlines()[0], most log viewers do the same) showedODataError:with nothing after it, andlogging.error("...: %s", err)printed a blank line followed by an indented block.The string now leads with the message:
primary_messagewhen the subclass defines it, elsemessage, else the class name; the status code follows in parentheses on the same line, and the nested error payload stays on a second line so nothing that was printed before is lost. For the throttling example from the issue:This matches the dotnet abstraction, where the generated error overrides
MessagewithError?.Message.Related Issue
Fixes #723
Notes
The base class knows neither
primary_messagenorerror, so both are read withgetattrand are optional; a plainAPIError(message="boom")printsboom, and one with nothing set printsAPIError (status 502). Thegetattr(self, "error", None)guard from #133 is kept.Testing Instructions
cd packages/abstractions && pytest tests/test_api_error.py: five cases covering a bare message, the status suffix, the class-name fallback, a generated-style subclass witherrorandprimary_message, and that no variant starts with a blank line. All five fail on main..pylintrcoption warnings.