Skip to content

Conversation

Guest0x0
Copy link
Contributor

the latest release of MoonBit introduces primitive for converting Error to json. This PR introduces this primitive to core

Copy link

peter-jerry-ye-code-review bot commented May 30, 2025

Missing documentation for the new ToJson implementation

Category
Maintainability
Code Snippet
pub impl ToJson for Error with to_json(self) = "%error.to_json"
Recommendation
Add documentation explaining the JSON format and behavior for different error types
Reasoning
Documentation is crucial for understanding how errors are serialized, especially since the test shows different behaviors for errors with and without ToJson implementation

TODO comment in test code indicates potential user experience issues

Category
Maintainability
Code Snippet
//TODO: note despite impl ToJson for Error, for suberror
// we can not use @json.inspect to inspect the error
Recommendation
Either implement the suggested auto-derivation for suberrors or document this limitation clearly in the main Error documentation
Reasoning
The current behavior might be unintuitive for users, as they cannot use json.inspect directly on suberrors despite Error implementing ToJson

Test coverage could be improved

Category
Correctness
Code Snippet
test "error to json" {
fn j(err : Error) {
err.to_json()
}
Recommendation
Add test cases for nested errors and more complex error scenarios to ensure proper serialization
Reasoning
Current tests only cover basic cases with simple integer payloads. Testing more complex scenarios would help ensure the robustness of the implementation

@peter-jerry-ye peter-jerry-ye requested a review from bobzhang May 30, 2025 03:41
@bobzhang bobzhang force-pushed the Guest0x0/add-error-tojson branch from 51220e2 to f08202d Compare May 30, 2025 09:17
@bobzhang bobzhang enabled auto-merge (rebase) May 30, 2025 09:17
@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 7005

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 92.518%

Totals Coverage Status
Change from base Build 7003: 0.01%
Covered Lines: 8742
Relevant Lines: 9449

💛 - Coveralls

@bobzhang bobzhang force-pushed the Guest0x0/add-error-tojson branch from f08202d to f69cfa2 Compare May 31, 2025 07:37
@bobzhang
Copy link
Contributor

note despite impl ToJson for Error, for suberror
we can not use @json.inspect to inspect the error

 @json.inspect(ErrWithoutToJson(42)) // type error

This also applies to impl Show for Error, for suberror
we can not use inspect to inspect the error
we may auto-derive ToJson and Show for suberror automatically
to enhance the user experience

@bobzhang bobzhang disabled auto-merge May 31, 2025 07:44
@bobzhang bobzhang merged commit b5261eb into main May 31, 2025
11 of 12 checks passed
@bobzhang bobzhang deleted the Guest0x0/add-error-tojson branch May 31, 2025 07:44
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.

3 participants