Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Oct 31, 2025

Implemented ToStringView for Char to enable Array::join on Array[Char]:

  • Add ToStringView implementation for Char in string/string_like.mbt
  • Add test to verify Array::join works with Array[Char]
  • Add comprehensive test demonstrating various use cases
  • Apply suggestion from @hackwaly to use [self] syntax
  • Fix formatting issues (trailing whitespace)
  • Run moon fmt and moon info to update interfaces
  • Run moon test to verify all tests pass (5394 tests passed)
  • Run moon check to verify code quality

Changes made:

  • Added pub impl ToStringView for Char in string/string_like.mbt using [self] syntax
  • Added test in array/array_test.mbt to verify basic functionality
  • Added comprehensive test in string/tostringview_char_demo_test.mbt demonstrating:
    • Simple join without separator
    • Join with separator
    • Empty array handling
    • Single character handling
    • Unicode character support
  • Updated pkg.generated.mbti to reflect the new implementation
  • Fixed formatting issues to pass CI checks

The implementation allows using Array::join on Array[Char] as requested in the issue.

Original prompt

This section details on the original issue you should resolve

<issue_title>Impl ToStringView for Char</issue_title>
<issue_description>Is your feature request related to a problem? Please describe.
A clear and concise description of what the problem is. Ex. I'm always frustrated when [...]

I want to use Array::join on Array[Char].

Describe the solution you'd like
A clear and concise description of what you want to happen.

Impl ToStringView for Char

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

Additional context
Add any other context or screenshots about the feature request here.

  • I do think that this should be part of the core library and should be delivered to all the MoonBit developers.
    </issue_description>

Comments on the Issue (you are @copilot in this section)


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

@coveralls
Copy link
Collaborator

coveralls commented Oct 31, 2025

Pull Request Test Coverage Report for Build 1745

Details

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.001%) to 89.817%

Totals Coverage Status
Change from base Build 1743: 0.001%
Covered Lines: 9985
Relevant Lines: 11117

💛 - Coveralls

@peter-jerry-ye-code-review
Copy link

peter-jerry-ye-code-review bot commented Oct 31, 2025

Test file naming inconsistency with existing conventions

Category
Maintainability
Code Snippet
string/tostringview_char_demo_test.mbt
Recommendation
Consider renaming to follow existing test naming patterns like string_like_test.mbt or integrating the comprehensive tests into the existing array_test.mbt file where the basic functionality is already tested
Reasoning
Having a separate demo test file for a single trait implementation creates unnecessary file proliferation. The comprehensive tests could be consolidated with existing test files to maintain better organization and discoverability.

Duplicated test logic between files

Category
Maintainability
Code Snippet
// In array_test.mbt:
let chars : Array[Char] = ['h', 'e', 'l', 'l', 'o']
let result = chars.join("")
inspect(result, content="hello")

// In tostringview_char_demo_test.mbt:
let chars1 : Array[Char] = ['h', 'e', 'l', 'l', 'o']
let result1 = chars1.join("")
inspect(result1, content="hello")
Recommendation
Remove the basic functionality tests from array_test.mbt and keep only the comprehensive tests in the dedicated test file, or vice versa. Avoid testing the same basic scenario in multiple places
Reasoning
Testing the exact same functionality (['h', 'e', 'l', 'l', 'o'].join("")) in two different files creates maintenance overhead and doesn't add value. One comprehensive test suite is better than scattered duplicated tests.

Implementation appears correct and well-documented

Category
Correctness
Code Snippet
pub impl ToStringView for Char with to_string_view(self) -> StringView {
[self]
}
Recommendation
The implementation is correct. The [self] syntax properly creates a StringView from a single character as suggested by @hackwaly
Reasoning
The implementation correctly uses the recommended syntax and the documentation clearly explains the purpose. The trait implementation enables the desired functionality for Array::join operations.

@peter-jerry-ye-code-review
Copy link

Inefficient double conversion from Char to String to StringView

Category
Performance
Code Snippet
pub impl ToStringView for Char with to_string_view(self) -> StringView {
self.to_string()[:]
}
Recommendation
Consider implementing a direct Char to StringView conversion if available in the MoonBit standard library, or add a comment explaining why the double conversion is necessary
Reasoning
The current implementation first converts Char to String (likely allocating memory) and then creates a StringView from it. This could be inefficient for operations on large arrays of characters, as it creates intermediate String objects that may not be necessary.

Missing comprehensive test coverage for edge cases

Category
Maintainability
Code Snippet
test "Array::join with Char" {
let chars : Array[Char] = ['h', 'e', 'l', 'l', 'o']
let result = chars.join("")
inspect(result, content="hello")

let chars2 : Array[Char] = ['a', 'b', 'c']
let result2 = chars2.join(", ")
inspect(result2, content="a, b, c")
}
Recommendation
Add test cases for edge scenarios: empty array, single character array, special characters (unicode, whitespace), and null characters if supported
Reasoning
The current tests only cover basic happy path scenarios. Edge cases should be tested to ensure robustness, especially since this is a core library feature that will be used by many developers.

Potential memory lifecycle concerns with StringView creation

Category
Correctness
Code Snippet
pub impl ToStringView for Char with to_string_view(self) -> StringView {
self.to_string()[:]
}
Recommendation
Verify that the StringView remains valid after the temporary String goes out of scope, or document the expected lifetime behavior
Reasoning
Creating a StringView from a temporary String object raises questions about memory safety. The StringView might reference memory that could be deallocated when the temporary String is garbage collected, depending on MoonBit's memory management model.

Copilot AI changed the title [WIP] Implement ToStringView for Char Implement ToStringView for Char Oct 31, 2025
Copilot AI requested a review from hackwaly October 31, 2025 06:00
Copilot finished work on behalf of hackwaly October 31, 2025 06:00
@hackwaly
Copy link
Contributor

@copilot fix CI failure.

Copy link
Contributor Author

Copilot AI commented Oct 31, 2025

@copilot fix CI failure.

Fixed in commit 933ff09. The CI failure was caused by trailing whitespace in blank lines that moon fmt removes. The formatting has been corrected.

Copilot AI requested a review from hackwaly October 31, 2025 10:29
Copilot finished work on behalf of hackwaly October 31, 2025 10:29
@hackwaly hackwaly marked this pull request as ready for review October 31, 2025 13:39
@hackwaly hackwaly force-pushed the copilot/impl-tostringview-for-char branch from 933ff09 to fd73a83 Compare October 31, 2025 13:39
@hackwaly hackwaly requested a review from Yu-zh November 1, 2025 02:07
@hackwaly hackwaly force-pushed the copilot/impl-tostringview-for-char branch from 28829ab to 662fc26 Compare November 2, 2025 14:08
@hackwaly hackwaly enabled auto-merge (squash) November 2, 2025 14:08
@hackwaly hackwaly merged commit 279a9dc into main Nov 2, 2025
10 checks passed
@hackwaly hackwaly deleted the copilot/impl-tostringview-for-char branch November 2, 2025 14:13
hackwaly added a commit that referenced this pull request Nov 3, 2025
hackwaly added a commit that referenced this pull request Nov 3, 2025
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.

Impl ToStringView for Char

4 participants