Only try to fixup broken AST issue 220 - #221
Conversation
|
Note, for some god forsaken reason, github is not visualizing the whitespace properly on this pr, so if it seems a little strange, do a git diff/show manually |
|
Ok, I fixed all the tests, (previously I only ran |
There was a problem hiding this comment.
🟡 Changes recommended
Missing-comma repair can still run against valid AST regions through other repair detectors.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Restricts missing-record-comma detection to malformed ASTs and adds regression coverage for issue #220.
Changes:
- Gates comma-repair detection on parse errors or garbage nodes.
- Adds nested-interpolation formatting and idempotency fixtures.
File summaries
| File | Description |
|---|---|
src/formatting/mod.rs |
Gates missing-comma detection. |
tests/ground_truth.rs |
Registers regression tests. |
tests/fixtures/input/nested_interpolation_issue220.nu |
Adds regression input. |
tests/fixtures/expected/nested_interpolation_issue220.nu |
Defines expected formatting. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Only try to detect fixable errors if the AST is broken | ||
| if has_parsing_malformed_spans || has_garbage { | ||
| malformed_spans.extend(detect_missing_record_comma_spans(&source_text)); |
| ( | ||
| "nested_interpolation_issue220", | ||
| ground_truth_nested_interpolation_issue220, | ||
| idempotencynested_interpolation_issue220 |
Description of changes
There are "detectors of broken code", that were used to fixup trivial issues. An issue with that is that they are not perfect, and are far more likely to be broken in an unexpected way. So this change only runs them, when the AST is broken.
Although this PR doesn't address the root cause of #220, it is the correct thing to do in the long run anyway.
Added a test, that would break because
detect_missing_record_comma_spanswould detect an error where one didn't exist. Fixed by not running it on good AST.Relevant Issues
#220