moonbit-docs icon indicating copy to clipboard operation
moonbit-docs copied to clipboard

fix(next/diff): String::split API breaking change

Open illusory0x0 opened this issue 1 year ago • 2 comments

illusory0x0 avatar Apr 25 '25 00:04 illusory0x0

Ensure string ownership with explicit to_string() conversion

Category Correctness Code Snippet lines.push(Line::new(line_number, line.to_string())) Recommendation The current change is correct. Keep the explicit to_string() conversion to ensure proper string ownership. Reasoning The split() operation likely returns string slices/views. When constructing Line objects that need to own their content, explicit conversion to owned strings prevents potential lifetime/ownership issues.

Duplicated code across multiple files

Category Maintainability Code Snippet Files: part1/line.mbt, part2/line.mbt, part3/line.mbt contain identical code Recommendation Consider refactoring the common line handling logic into a shared module that can be used by all three parts Reasoning Having identical code in multiple places increases maintenance burden and risk of inconsistencies when changes are needed. A shared implementation would be easier to maintain and ensure consistency.

Missing documentation for Line constructor and lines function

Category Maintainability Code Snippet fn lines(str : String) -> Array[Line] { Recommendation Add documentation comments explaining the purpose and behavior of the Line::new constructor and lines function Reasoning Documentation helps other developers understand the purpose and usage of the code, especially for public functions that likely form part of the module's API

I would rather change it the other way round: make Line.text @string.View

peter-jerry-ye avatar Apr 25 '25 03:04 peter-jerry-ye