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

chore: rename deep_clone into copy #1550

Merged
merged 5 commits into from
Jan 22, 2025
Merged

chore: rename deep_clone into copy #1550

merged 5 commits into from
Jan 22, 2025

Conversation

GMH233
Copy link
Contributor

@GMH233 GMH233 commented Jan 22, 2025

#1461
Basically, just change

pub fn deep_clone[V](self : T[V]) -> T[V] {
  *some code*
}

To

/// @alert deprecated "Use `copy` instead"
/// @coverage.skip
pub fn deep_clone[V](self : T[V]) -> T[V] {
  copy(self)
}
pub fn copy[V](self : T[V]) -> T[V] {
  *some code*
}

Copy link

peter-jerry-ye-code-review bot commented Jan 22, 2025

‼️ This code review is generated by a bot. Please verify the content before trusting it.

From the git diff provided, I can identify the following potential issues:

  1. Inconsistent Function Naming: The codebase is transitioning from deep_clone to copy, but there might be other parts of the codebase still using deep_clone. This could lead to confusion and maintainability issues. While the deprecated annotation is added, existing code might still be using the old function name.

  2. Shallow Copy Behavior: The copy function (previously deep_clone) is explicitly noted to only copy the tree structure and not the values. This could be problematic if users expect a true deep copy of the data structure. The comment states "It requires a Clone trait on T, which we don't have yet" - this limitation should be addressed in future updates.

  3. Multiple Ways to Calculate Difference: The code shows both diff and difference methods, with diff being marked as deprecated. Having two methods for the same operation could lead to confusion and inconsistency in code using this library. The deprecated method should eventually be removed to avoid confusion.

These observations suggest a transition period in the codebase where older patterns are being updated to newer ones, which could temporarily impact code clarity and consistency.

@coveralls
Copy link
Collaborator

coveralls commented Jan 22, 2025

Pull Request Test Coverage Report for Build 5014

Details

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 85.007%

Totals Coverage Status
Change from base Build 5011: 0.0%
Covered Lines: 5222
Relevant Lines: 6143

💛 - Coveralls

@GMH233 GMH233 marked this pull request as ready for review January 22, 2025 13:08
@bobzhang bobzhang added this pull request to the merge queue Jan 22, 2025
@bobzhang bobzhang removed this pull request from the merge queue due to a manual request Jan 22, 2025
@bobzhang bobzhang added this pull request to the merge queue Jan 22, 2025
Merged via the queue into moonbitlang:main with commit 05c41bd Jan 22, 2025
13 of 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