Skip to content

Conversation

Guest0x0
Copy link
Contributor

Since MoonBit allows calling impl with dot syntax now, it is no longer necessary to define to_string method wrappers to support .to_string() usage. This PR remove such unnecessary to_string wrapper methods.

One of the removed methods, Result::to_string in @result, actually has wrong behavior, forgetting to escape its arguments when necessary. This PR fixes the issue.

I also added tests for the Show impls.

Copy link

peter-jerry-ye-code-review bot commented Jan 16, 2025

‼️ This code review is generated by a bot. Please verify the content before trusting it.

Here are three potential issues or observations from the provided git diff output:

  1. Removal of to_string Methods:

    • The to_string method has been removed from several implementations, such as ArrayView, Bytes, List, and Result. This could break existing code that relies on these methods for converting objects to their string representations. If these methods were intentionally removed, ensure that alternative ways to achieve the same functionality are provided or documented.
  2. Inconsistent String Representation in Tests:

    • In the Show for String test, there is an inconsistency in the expected output for Show::to_string("---\"---'---\\---\n---\t---\x67---"). The test expects the output to be:
      #|---"---'---\---
      #|---	---g---
      
      However, the actual implementation might not produce this exact output, especially with escape sequences like \n and \t. Ensure that the test expectations align with the actual behavior of the to_string method.
  3. Potential Redundancy in Show Implementation:

    • The Show trait implementation for Bytes and BytesView includes a method output that writes to a logger. However, the to_string method, which was removed, previously used this output method to build a string. If to_string is no longer available, ensure that the output method is sufficient for all use cases or that an alternative method for string conversion is provided.

These observations highlight potential areas where the changes might introduce issues or require additional attention to maintain functionality and consistency.

@Guest0x0 Guest0x0 force-pushed the Guest0x0/fix-some-to-string branch from 76962db to 4682573 Compare January 16, 2025 03:20
@coveralls
Copy link
Collaborator

coveralls commented Jan 16, 2025

Pull Request Test Coverage Report for Build 4792

Details

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

Totals Coverage Status
Change from base Build 4784: 0.2%
Covered Lines: 4872
Relevant Lines: 5861

💛 - Coveralls

@Guest0x0 Guest0x0 force-pushed the Guest0x0/fix-some-to-string branch from 4682573 to 3d45cd6 Compare January 16, 2025 03:22
@Guest0x0 Guest0x0 force-pushed the Guest0x0/fix-some-to-string branch from 3d45cd6 to 105b0f0 Compare January 16, 2025 04:03
@bobzhang bobzhang merged commit 99cfedd into main Jan 16, 2025
14 checks passed
@bobzhang bobzhang deleted the Guest0x0/fix-some-to-string branch January 16, 2025 05:57
@bobzhang
Copy link
Contributor

@Guest0x0 it seems that we also need migrate from to_json method to trait method

lynzrand pushed a commit to SyoujyoujiNaiki/moonbit-core that referenced this pull request Jan 22, 2025
* remove some unnecessary/wrong [to_string] methods

* add test
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.

4 participants