Skip to content
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

Rename write_sub_string to write_substring #1139

Merged
merged 4 commits into from
Oct 18, 2024
Merged

Conversation

LuoTat
Copy link
Contributor

@LuoTat LuoTat commented Oct 17, 2024

This PR addresses the issue #1110

This PR rename all the write_sub_string to write_substring.

Copy link

peter-jerry-ye-code-review bot commented Oct 17, 2024

From the provided git diff output, I observe the following potential issues or suggestions:

  1. Renaming Function without Updating All References:

    • The function write_sub_string was renamed to write_substring, but there are instances where the old name write_sub_string is still used in the codebase. For example:
      -pub fn write_sub_string(
      +pub fn write_substring(
      However, in the test files and some implementation files, the old name write_sub_string is still being used:
      -test "write_sub_string method" {
      +test "write_substring method" {
      This could lead to confusion and potential runtime errors if not all references are updated.
  2. Deprecation Alert without Proper Annotation:

    • The function write_sub_string is marked as deprecated with a doc comment:
      /// @alert deprecated "Use `Buffer::write_substring` instead"
      pub fn write_sub_string(
      
      However, this deprecation alert should ideally be consistently applied across all relevant files to ensure developers are aware of the deprecation.
  3. Missing Updates in Implementation Files:

    • The renaming from write_sub_string to write_substring should also be reflected in the implementation files. For example, in builtin/builtin.mbti, the function write_sub_string is marked as deprecated but the corresponding method in the implementation should be updated:
      -write_sub_string(Self, String, Int, Int) -> Unit //deprecated
      +write_substring(Self, String, Int, Int) -> Unit
      Additionally, the trait implementation for Logger should be updated to reflect the new function name:
      -impl Logger::write_sub_string
      +impl Logger::write_substring

To address these issues, ensure that all references to write_sub_string are updated to write_substring and that the deprecation alert is consistently applied. This will help maintain code consistency and avoid potential runtime errors or confusion.

@coveralls
Copy link
Collaborator

coveralls commented Oct 17, 2024

Pull Request Test Coverage Report for Build 3645

Details

  • 2 of 7 (28.57%) changed or added relevant lines in 6 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.05%) to 82.718%

Changes Missing Coverage Covered Lines Changed/Added Lines %
buffer/buffer.mbt 0 1 0.0%
builtin/buffer.mbt 0 1 0.0%
builtin/traits.mbt 0 1 0.0%
coverage/coverage.mbt 0 2 0.0%
Totals Coverage Status
Change from base Build 3643: -0.05%
Covered Lines: 4255
Relevant Lines: 5144

💛 - Coveralls

@hackwaly
Copy link
Contributor

Great! Thanks.

Could you please keep write_sub_string exist and mark it as deprecated first. We'd like to remove them in future.

@hackwaly hackwaly merged commit 622edb9 into moonbitlang:main Oct 18, 2024
12 of 13 checks passed
@LuoTat LuoTat deleted the fix branch October 18, 2024 16:25
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