Skip to content

refactor: deprecate Show for Option, migrate to Debug#3436

Open
bobzhang wants to merge 5 commits intomainfrom
hongbo/deprecate_option
Open

refactor: deprecate Show for Option, migrate to Debug#3436
bobzhang wants to merge 5 commits intomainfrom
hongbo/deprecate_option

Conversation

@bobzhang
Copy link
Copy Markdown
Contributor

@bobzhang bobzhang commented Apr 17, 2026

Summary

  • Deprecate Show impl for Option and Option::to_string — Option has no meaningful Display representation (same reasoning as Rust's stdlib)
  • Migrate all call sites from inspect to debug_inspect or guard ... is patterns
  • Add Debug impls for JsonDecodeError, JsonPath, and @cmp.Reverse
  • Update expected test content strings for Debug output format differences (StringView, ArrayView, Byte, etc.)

Test plan

  • moon check passes (0 errors, 77 remaining deprecation warnings from structural uses)
  • moon test passes (6319/6319 tests)

🤖 Generated with Claude Code


Open with Devin

Show (Display) for Option has no meaningful string representation,
following the same reasoning as Rust's stdlib. Deprecate both the
Show impl for Option and Option::to_string. Migrate all call sites
from inspect to debug_inspect or guard patterns. Add Debug impls
for JsonDecodeError, JsonPath, and Reverse.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@coveralls
Copy link
Copy Markdown
Collaborator

coveralls commented Apr 17, 2026

Coverage Report for CI Build 3840

Coverage decreased (-0.2%) to 94.692%

Details

  • Coverage decreased (-0.2%) from the base build.
  • Patch coverage: 32 uncovered changes across 4 files (9 of 41 lines covered, 21.95%).
  • 6 coverage regressions across 1 file.

Uncovered Changes

File Changed Covered %
builtin/linked_hash_map.mbt 13 0 0.0%
set/linked_hash_set.mbt 11 0 0.0%
json/json_path.mbt 8 2 25.0%
builtin/autoloc.mbt 2 0 0.0%

Coverage Regressions

6 previously-covered lines in 1 file lost coverage.

File Lines Losing Coverage Coverage
builtin/show.mbt 6 90.0%

Coverage Stats

Coverage Status
Relevant Lines: 15505
Covered Lines: 14682
Line Coverage: 94.69%
Coverage Strength: 220656.52 hits per line

💛 - Coveralls

Copy link
Copy Markdown

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

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

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Open in Devin Review

bobzhang and others added 4 commits April 17, 2026 18:18
- Suppress deprecation warnings on structs with Option fields that
  derive Show (ArgsLoc, Entry, value_range)
- Remove unused Eq impl for Entry
- Add Debug impls for Reverse, JsonDecodeError, JsonPath
- Fix all remaining inspect→debug_inspect and assert_eq→@test.assert_eq
- Update expected test content strings for Debug format differences

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add deprecated inner() accessors for newtype structs that the stable
toolchain (0.1.20260409) generates but the newer local version omits.
Remove unsupported #deprecated annotation on impl in mbti.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 1 new potential issue.

View 9 additional findings in Devin Review.

Open in Devin Review

Comment thread argparse/value_range.mbt
///|
/// Number-of-values constraint for an argument.
#warnings("-deprecated_syntax")
#warnings("-deprecated_syntax-deprecated")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Likely malformed #warnings directive combining two flags with ambiguous syntax

The directive #warnings("-deprecated_syntax-deprecated") is the only instance in the entire codebase that attempts to combine two warning flags. Every other #warnings usage suppresses exactly one flag (e.g., #warnings("-deprecated") or #warnings("-deprecated_syntax")). The concatenated format -deprecated_syntax-deprecated is ambiguous — it may be parsed as a single unrecognized warning name deprecated_syntax-deprecated rather than two separate flags. The struct ValueRange has a field upper : Int? and derive(Eq, Show), so it needs both -deprecated_syntax (for the old struct syntax) and -deprecated (because Show for Int? is now deprecated). If neither flag is properly suppressed, the code would emit warnings.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants