Skip to content

Conversation

Yu-zh
Copy link
Collaborator

@Yu-zh Yu-zh commented Apr 15, 2025

Part of #1914. This PR only adds methods to View, and does not change other public APIs.

Copy link

peter-jerry-ye-code-review bot commented Apr 15, 2025

Several methods contain TODO comments about removing .view() calls

Category
Maintainability
Code Snippet
// todo: remove .view() in new version
String::from_array(chars).view()
Recommendation
Either remove the .view() calls now if possible, or create a tracking issue for this technical debt. Add justification in comments why this needs to be done later.
Reasoning
TODO comments tend to be forgotten. Having a clear plan for addressing these in future helps maintain code quality and provides context for other developers.

iter2() function has complex loop control flow that could be error-prone

Category
Correctness
Code Snippet
continue index + 2, n + 1
Recommendation
Consider splitting the surrogate pair handling into a separate helper function to make the logic more clear and testable:

fn handle_surrogate_pair(view: View, index: Int, n: Int) -> (Int, Int, Char)? {...}```
**Reasoning**
The current implementation mixes multiple concerns: iteration, surrogate pair detection, and yield control. Separating these would make the code more maintainable and easier to verify for correctness.

</details>
<details>

<summary> Documentation is missing for iter2() method </summary>

**Category**
Maintainability
**Code Snippet**
///|
pub fn View::iter2(self : View) -> Iter2[Int, Char]
**Recommendation**
Add documentation explaining the purpose of iter2(), especially how it differs from iter(). Include examples if possible.
**Reasoning**
All other public methods have documentation explaining their purpose. Consistent documentation helps users understand the API better.

</details>

@coveralls
Copy link
Collaborator

coveralls commented Apr 15, 2025

Pull Request Test Coverage Report for Build 6197

Details

  • 14 of 18 (77.78%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.02%) to 91.56%

Changes Missing Coverage Covered Lines Changed/Added Lines %
string/view.mbt 14 18 77.78%
Totals Coverage Status
Change from base Build 6196: -0.02%
Covered Lines: 5641
Relevant Lines: 6161

💛 - Coveralls

@Yu-zh Yu-zh requested a review from bobzhang April 15, 2025 02:43
@bobzhang bobzhang force-pushed the stringview-methods branch from bbeda91 to 02184c9 Compare April 15, 2025 08:01
@bobzhang bobzhang enabled auto-merge (rebase) April 15, 2025 08:06
@bobzhang bobzhang merged commit 3dab002 into moonbitlang:main Apr 15, 2025
14 checks passed
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.

3 participants