feat(arxjit): add structured diagnostics and the public error hierarchy#95
feat(arxjit): add structured diagnostics and the public error hierarchy#95Jaskirat-s7 wants to merge 1 commit into
Conversation
Add arxjit.diagnostics (frozen Diagnostic record + DiagnosticSeverity, same 6-field contract as pyarx) and arxjit.errors (ArxJitError base carrying a list of diagnostics, with SourceUnavailableError and UnsupportedSyntaxError for the Sprint 2 source-extraction and validation layers). Export both from the package root; 15 new tests.
08a65a0 to
d81cb52
Compare
xmnlab
left a comment
There was a problem hiding this comment.
Review verdict: request changes
The implementation is clean and CI is green, but I found one public-API issue that should be resolved before merging.
Blocking: define how AST columns are normalized
Diagnostic.column is documented as a one-based source column. However, the upcoming validation layer will obtain locations from Python AST nodes, whose col_offset is a zero-based UTF-8 byte offset, not a one-based character column. ([Python documentation][1])
Simply storing node.col_offset violates the documented contract. Adding one fixes ASCII positions but can still produce incorrect visual columns when non-ASCII characters precede the node.
Because this PR intentionally establishes the public diagnostic contract before extraction and validation land, I recommend choosing one precise representation now:
- Prefer a one-based Unicode character column for human-readable diagnostics.
- Convert AST byte offsets at the boundary rather than exposing them directly.
- Add future integration tests covering an item at column zero and an item preceded by a non-ASCII character.
- Alternatively, document it explicitly as a zero-based UTF-8 byte offset, though that is less suitable for the current human-readable
__str__output.
Non-blocking: clarify SourceUnavailableError
The class title says it represents source that “cannot be obtained,” but its summary also covers source that was obtained but cannot be parsed as a standalone function definition, including lambdas. Meanwhile, UnsupportedSyntaxError represents rejected Python constructs. This makes the boundary between these public exception types unclear.
Consider either:
- renaming it to
SourceExtractionError, or - reserving it for genuinely unavailable source and treating unsupported callable shapes as
UnsupportedSyntaxError.
What looks good
The frozen diagnostic value object, defensive copying of diagnostic sequences, explicit package exports, and small exception hierarchy are straightforward and consistent with the existing arxjit pattern.
All three associated Actions workflows completed successfully.
What
First PR of arxjit Sprint 2 (source extraction + Python AST validation), per the 4-PR split posted in the Discord thread:
arxjit.diagnostics— frozenDiagnosticrecord (severity, message, filename, line, column, code) +DiagnosticSeverityenum (ERROR/WARNING/INFO/HINT), with a human-readable__str__(file:line:col: error: [code] message).arxjit.errors—ArxJitErrorbase carryinglist[Diagnostic], withSourceUnavailableError(source cannot be retrieved and parsed as a standalone function definition: REPL,exec, C functions, lambdas) andUnsupportedSyntaxError(constructs outside the v1 subset), mapping 1:1 to the two Sprint 2 layers.Why diagnostics before extraction/validation
Both upcoming layers report user-facing failures — extraction fails on source-less functions, and validation's whole job is rejecting unsupported syntax with clear messages (wiki Issue 4). Landing the shared vocabulary first (~90 lines) means both PRs raise the same typed errors from day one instead of ad-hoc strings retrofitted later.
Design notes
astnodes.code(e.g.AJ001) is optional and unassigned for now; the validation PR can introduce stable codes if wanted.strict-style parameter on@jitto force-raise — that lands in the validation wiring PR, where these exceptions get raised.Checks
mypy src), bandit, vulture, mccabe: cleandouki syncrun and idempotent (second run: 0 updated)