Skip to content

feat: add JsonPointer support and conversions from and to json - #2763

Closed
Erchiusx wants to merge 15 commits into
moonbitlang:mainfrom
Erchiusx:json.pointer
Closed

feat: add JsonPointer support and conversions from and to json#2763
Erchiusx wants to merge 15 commits into
moonbitlang:mainfrom
Erchiusx:json.pointer

Conversation

@Erchiusx

Copy link
Copy Markdown
Contributor

next move: add URI support and try to replace JsonPath with JsonPointer

trying to address issue #2758

@peter-jerry-ye peter-jerry-ye left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The overall concept looks fine-ish, but the implementation doesn't look very idiomatic

Comment thread json/json_pointer.mbt Outdated

///|
pub fn JsonPointer::apply(self : JsonPointer, value : Json) -> Json? {
match (self, value) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be a loop at least?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

loop needs intermediate result collection, I think the cost is generally alike;
but it is true that implementations need improvement

I will use iterator to implement this apply, and I am considering to reverse the link direction as a refactor.

bobzhang and others added 11 commits September 25, 2025 15:32
Co-authored-by: peter-jerry-ye <16731997+peter-jerry-ye@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: peter-jerry-ye <16731997+peter-jerry-ye@users.noreply.github.com>
@peter-jerry-ye-code-review

Copy link
Copy Markdown
UTF-8 decoding function has been rewritten with potential performance regression

Category
Performance
Code Snippet
encoding/utf8/decode.mbt lines 25-135: The decode function was changed from StringBuilder-based approach to a manual byte array manipulation approach
Recommendation
Consider benchmarking the new implementation against the old one. The new approach may be more complex and harder to maintain without clear performance benefits. If performance is critical, add inline documentation explaining the optimization strategy.
Reasoning
The new implementation is significantly more complex with manual byte manipulation and could introduce bugs. Without benchmarks, it's unclear if this change provides actual performance benefits.

Bytes Show implementation removed from builtin/show.mbt but added in bytes/view.mbt may cause inconsistent behavior

Category
Correctness
Code Snippet
builtin/show.mbt lines 83-91 (removed) and bytes/view.mbt lines 548-552 (added)
Recommendation
Ensure the new Show implementation for Bytes in bytes/view.mbt provides exactly the same behavior as the removed one. Add tests to verify backward compatibility.
Reasoning
Moving Show implementation between modules could break existing code that depends on the specific formatting behavior, especially since the display format has changed significantly.

JsonPointer implementation lacks comprehensive error handling and documentation

Category
Maintainability
Code Snippet
json/json_pointer.mbt lines 1-313: New JsonPointer enum and related functions
Recommendation
Add more comprehensive documentation explaining RFC6901 compliance, edge cases, and error conditions. Consider adding more specific error types instead of using generic JsonPointerParsingError for all parsing failures.
Reasoning
The JsonPointer implementation is complex and deals with string parsing/escaping which is error-prone. Better documentation and error handling will make it more maintainable and easier to debug issues.

@Erchiusx

Copy link
Copy Markdown
Contributor Author

I see #2764 implemented another JSON pointer feature
Seemingly more integrated, maybe this PR can be closed now.

@peter-jerry-ye

Copy link
Copy Markdown
Collaborator

Yes, we will migrate the currently existing JsonPath to JsonPointer. Maybe we'll revisit this issue to see how to implement it closer to RFC. Thank you for your contribution.

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.

5 participants