8000 Breaking Change: make String::op_get return Int by Yu-zh · Pull Request #1861 · moonbitlang/core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Breaking Change: make String::op_get return Int #1861

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 12, 2025
Merged

Breaking Change: make String::op_get return Int #1861

merged 1 commit into from
May 12, 2025

Conversation

Yu-zh
Copy link
Collaborator
@Yu-zh Yu-zh commented Apr 1, 2025

No description provided.

@peter-jerry-ye-code-review peter-jerry-ye code review
Copy link
peter-jerry-ye-code-review bot commented Apr 1, 2025 •
edited
Loading
Documentation and test case mismatch for String::op_get

Category
Correctness
Code Snippet
test "String::op_get" {
let s = "Hello, 世界!"
inspect!(s[0].to_char(), content="Some('H')")...
Recommendation
Update the documentation to clearly state that the returned integer is a UTF-16 code unit value that may need to be converted to a character using to_char(). Also add a test case demonstrating behavior with surrogate pairs.
Reasoning
The current documentation and test cases don't fully explain the implications of returning a UTF-16 code unit. Users need to understand that some characters (like emoji or characters outside the BMP) may require handling multiple code units.

Potentially confusing API design with multiple character access methods

Category
Maintainability
Code Snippet
fn String::op_get(self : String, idx : Int) -> Int
fn String::get(String, Int) -> Char
Recommendation
Consider deprecating one of the methods or renaming op_get to something more descriptive like get_charcode or get_utf16_unit to make the difference more explicit.
Reasoning
Having both op_get and get methods with different return types but similar purposes could be confusing for users. The naming should clearly indicate the difference in behavior.

Missing documentation about return value range

Category
Maintainability
Code Snippet
/// Retrieves the charcode (UTF-16 code unit) at the specified index in a string.
Recommendation
Add documentation specifying the range of possible return values (0-65535) and explaining what they represent in terms of Unicode code points.
Reasoning
For a low-level string operation that returns raw UTF-16 values, it's important to document the possible range of return values and their meaning to prevent misuse and confusion.

@coveralls
Copy link
Collaborator
coveralls commented Apr 1, 2025 •
edited
Loading

Pull Request Test Coverage Report for Build 6651

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 92.451%
Totals Coverage Status
Change from base Build 6649: 0.0%
Covered Lines: 6136
Relevant Lines: 6637
💛 - Coveralls

@Yu-zh Yu-zh force-pushed the change-type-of-string-op-get branch from 59d61c2 to 67deb0e Compare April 1, 2025 14:02
@Yu-zh Yu-zh marked this pull request as draft April 1, 2025 14:07
@Yu-zh Yu-zh force-pushed the change-type-of-string-op-get branch from 67deb0e to cb61835 Compare April 2, 2025 07:22
@Yu-zh Yu-zh marked this pull request as ready for review April 7, 2025 03:41
@Yu-zh Yu-zh marked this pull request as draft April 7, 2025 06:26
@Yu-zh Yu-zh force-pushed the change-type-of-string-op-get branch from 168961c to ad416ee Compare April 11, 2025 08:11
@Yu-zh Yu-zh marked this pull request as ready for review April 14, 2025 02:01
@bobzhang bobzhang force-pushed the change-type-of-string-op-get branch from e4cb96d to b1f5574 Compare April 15, 2025 08:25
@Yu-zh Yu-zh force-pushed the change-type-of-string-op-get branch from 0fd6ed9 to 71dc62f Compare May 12, 2025 02:54
@Yu-zh Yu-zh force-pushed the change-type-of-string-op-get branch from 71dc62f to aec45fb Compare May 12, 2025 03:04
@Yu-zh Yu-zh merged commit bc2508d into moonbitlang:main May 12, 2025
27 of 28 checks passed
@Yu-zh Yu-zh deleted the change-type-of-string-op-get branch May 12, 2025 03:20
@FlyCloudC FlyCloudC mentioned this pull request May 19, 2025
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
consistency review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Yu-zh @coveralls @peter-jerry-ye
0