Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support @json.inspect #338

Merged
merged 1 commit into from
Oct 9, 2024
Merged

feat: support @json.inspect #338

merged 1 commit into from
Oct 9, 2024

Conversation

lijunchen
Copy link
Contributor

@lijunchen lijunchen commented Sep 24, 2024

Related Issues

Type of Pull Request

  • Bug fix
  • New feature
    • I have already discussed this feature with the maintainers.
  • Refactor
  • Performance optimization
  • Documentation
  • Other (please describe):

Does this PR change existing behavior?

  • Yes (please describe the changes below)

  • No

Does this PR introduce new dependencies?

  • [] No
  • Yes (please check binary size changes)
    • increase ~0.2M

Checklist:

  • Tests added/updated for bug fixes or new features
  • Compatible with Windows/Linux/macOS

@lijunchen lijunchen changed the title Inspect json feat: support inspect_json Sep 24, 2024
Copy link

peter-jerry-ye-code-review bot commented Oct 8, 2024

Observations from git diff Output

  1. Dependency Changes:

    • A new version of the console crate (version 0.13.0) has been added. This version seems to have several dependencies including encode_unicode, lazy_static, libc, regex, terminal_size, unicode-width, winapi, and winapi-util.
    • The difflib crate (version 0.4.0) has been added, which is likely used for diffing functionality.
    • The json-structural-diff crate (version 0.1.0) has been added with features including "colorize". This crate is likely used for comparing JSON structures and highlighting differences.
  2. Configuration Changes:

    • In crates/moonbuild/Cargo.toml, the json-structural-diff crate has been added as a dependency with the version 0.1.0 and features set to include "colorize".
  3. Code Changes in expect.rs:

    • The BufferExpect and Target structs have gained a new field mode: Option<String>. This suggests that there might be a new mode or context being tracked for these structures.
    • The ExpectFailedRaw struct now includes a mode: Option<String> field, indicating that the mode might be relevant when handling or reporting expectation failures.
    • The Replace struct and its methods have been updated to include a mode: Option<String> field. This field is used to determine how to process the actual content when applying a patch.
    • The apply_patch function has been modified to handle different modes (None, Some("json"), and other modes). For the json mode, it directly pushes the actual content string to the output, suggesting a specific handling for JSON content. For unsupported modes, it raises an error.
    • The render_expect_fail function has been updated to handle JSON mode differences using the json_structural_diff crate. If the mode is json, it compares the expected and actual JSON structures and prints a colored diff.

Suggestions

  1. Dependency Management:

    • Ensure that the new dependencies (console, difflib, json-structural-diff) are necessary and align with the project's goals. These dependencies introduce additional complexity and should be justified.
  2. Code Maintenance:

    • The addition of the mode field in multiple structs indicates a new dimension of control or context. Ensure that this new field is consistently used and documented to avoid confusion or misuse.
    • The handling of different modes in the apply_patch function should be thoroughly tested, especially the json mode, to ensure it behaves as expected.
  3. Error Handling:

    • The error message for unsupported modes in apply_patch is informative, but consider adding more context or logging to help with debugging if unexpected modes are encountered.

These observations and suggestions aim to ensure that the changes align with the project's goals, maintain code consistency, and enhance error handling and debugging capabilities.

@lijunchen lijunchen changed the title feat: support inspect_json feat: support @json.inspect Oct 8, 2024
@lijunchen lijunchen marked this pull request as ready for review October 8, 2024 07:22
@lijunchen lijunchen merged commit b061a30 into main Oct 9, 2024
10 checks passed
@lijunchen lijunchen deleted the inspect-json branch October 9, 2024 02:24
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.

1 participant