8000 fix unused trait impl by Guest0x0 · Pull Request #2332 · moonbitlang/core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix unused trait impl #2332

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

Merged
merged 2 commits into from
Jun 24, 2025
Merged

fix unused trait impl #2332

merged 2 commits into from
Jun 24, 2025

Conversation

Guest0x0
Copy link
Contributor

Fix some unused trait implementation. Thanks to simplification of type system, unused warning for trait implementations is under development and will soonish be available. This PR fixes core.

Copy link
peter-jerry-ye-code-review bot commented Jun 24, 2025
The Eq trait implementation for Entry is moved to a test file which could cause confusion

Category
Maintainability
Code Snippet
impl[K : Eq, V] Eq for Entry[K, V] with op_equal(self, other) {
self.hash == other.hash && self.key == other.key
}
Recommendation
Consider keeping the Eq implementation in the main source file linked_hash_map.mbt if it's part of the public API. If it's only needed for tests, add a comment explaining this.
Reasoning
Moving implementations to test files can make the code harder to maintain and understand, as users would expect to find trait implementations in the main source files. If this implementation is truly only needed for tests, it should be clearly documented.

Tree Show implementation test cases could be more comprehensive

Category
Correctness
Code Snippet
test "Show for Tree" {
inspect((Empty : Tree[Int]), content="Empty")
...
}
Recommendation
Add test cases for more complex tree structures and edge cases, such as deeply nested trees or trees with mixed node types.
Reasoning
While the current test cases cover basic scenarios, tree structures can be complex and the Show implementation should be verified to work correctly for all possible tree configurations.

Removal of #coverage.skip annotation without explanation

Category
Maintainability
Code Snippet
/// Print the tree structure. For debug use only.

  • #coverage.skip
    impl[A : Show] Show for Tree[A] with output(self, logger : &Logger) {
    Recommendation
    Add a comment explaining why the coverage skip annotation was removed, or if this implementation should now be covered by tests.
    Reasoning
    Changes to coverage annotations should be documented to help maintain code quality standards and testing requirements. The removal might indicate a change in testing strategy that should be communicated.

@coveralls
Copy link
Collaborator
coveralls commented Jun 24, 2025

Pull Request Test Coverage Report for Build 7488

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 89.378%

Totals Coverage Status
Change from base Build 7484: 0.04%
Covered Lines: 3534
Relevant Lines: 3954

💛 - Coveralls

@bobzhang bobzhang enabled auto-merge (rebase) June 24, 2025 07:51
@bobzhang bobzhang merged commit a681a7b into main Jun 24, 2025
12 checks passed
@bobzhang bobzhang deleted the Guest0x0/remove-unused-impl branch June 24, 2025 07:57
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
0