Skip to content

refactor: update Show implementation for Char to use new syntax and improve output handling #2046

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bobzhang
Copy link
Contributor

@bobzhang bobzhang commented Apr 30, 2025

Error: Moonc.Basic_hash_string.Key_not_found("$@moonbitlang/core/builtin.Show::Char::to_string")

@bobzhang bobzhang requested a review from Yoorkin April 30, 2025 08:16
Copy link

Consolidation of intrinsic function implementation

Category
Maintainability
Code Snippet
pub impl Show for Char with to_string(self) = "%char.to_string"
Recommendation
The change is good, but consider adding a comment explaining that this is an intrinsic function implementation
Reasoning
While the new syntax is more concise, it's less immediately clear that this is an intrinsic function. A brief comment would help future maintainers understand the implementation without having to look up the syntax.

Documentation could be more descriptive

Category
Maintainability
Code Snippet
///| Convert Char to String
Recommendation
Enhance the documentation to: ///| Converts a Char to its String representation using the intrinsic implementation
Reasoning
The current documentation is very basic. Since this is a core conversion functionality, having more detailed documentation would be helpful for users of the library.

Removal of explicit implementation might affect behavior

Category
Correctness
Code Snippet

  • fn char_to_string(char : Char) -> String {
  • [char]
  • }
    Recommendation
    Verify that the intrinsic %char.to_string produces identical results to the previous implementation of converting a char to a single-character array
    Reasoning
    The old implementation explicitly showed the conversion logic (creating a single-character array). We should ensure the intrinsic function maintains the same behavior.

@bobzhang bobzhang marked this pull request as draft April 30, 2025 08:22
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