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

hongbo/tweak logger interface #1845

Merged
merged 3 commits into from
Mar 28, 2025
Merged

hongbo/tweak logger interface #1845

merged 3 commits into from
Mar 28, 2025

Conversation

bobzhang
Copy link
Contributor

  • refactor: remove deprecated functions and update buffer interface
  • remove deprecated trait method write_sub_string
  • make write_char defaultable

Copy link

peter-jerry-ye-code-review bot commented Mar 28, 2025

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

Based on the pull request message and the diff, this PR primarily focuses on refactoring the Logger trait and buffer interface by removing deprecated functionality and making some methods defaultable.

Let me break this down:

  1. Removal of deprecated function write_sub_string
  2. Adding a default implementation for write_char method that converts a single character to a string using write_string

Here's my review:

✅ [Good cleanup of deprecated method]
  • Category: Maintainability
  • Code Snippet:
    - write_sub_string(Self, String, Int, Int) -> Unit
    - /// @alert deprecated "use `Logger::write_substring` instead"
  • Recommendation: Good practice to remove deprecated methods to keep codebase clean
  • Reasoning: Removing deprecated methods reduces technical debt and prevents confusion for users of the API. The write_substring method serves the same purpose with a more consistent naming convention.
💡 [Default implementation of write_char could be more efficient]
  • Category: Performance
  • Code Snippet:
    impl Logger with write_char(self, value) {
      self.write_string([value])
    }
  • Recommendation: Consider a direct implementation for single characters instead of converting to string
  • Reasoning: Converting a single character to a string array might introduce unnecessary overhead. A direct implementation that handles the character directly would be more efficient.
💡 [Missing documentation for default implementation]
  • Category: Maintainability
  • Code Snippet:
    impl Logger with write_char(self, value) {
      self.write_string([value])
    }
  • Recommendation: Add documentation explaining this is a default implementation
  • Reasoning: Documenting default implementations helps developers understand the fallback behavior when no specific implementation is provided.

Overall, the changes are good housekeeping, but the default implementation of write_char could be improved for both performance and documentation.

@bobzhang bobzhang force-pushed the hongbo/tweak_logger_interface branch from aa362fd to e08b898 Compare March 28, 2025 19:28
@coveralls
Copy link
Collaborator

coveralls commented Mar 28, 2025

Pull Request Test Coverage Report for Build 5887

Details

  • 0 of 1 (0.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 92.417%

Changes Missing Coverage Covered Lines Changed/Added Lines %
builtin/traits.mbt 0 1 0.0%
Totals Coverage Status
Change from base Build 5886: 0.0%
Covered Lines: 5570
Relevant Lines: 6027

💛 - Coveralls

@bobzhang bobzhang enabled auto-merge (rebase) March 28, 2025 20:09
@bobzhang bobzhang merged commit 20772e0 into main Mar 28, 2025
11 of 12 checks passed
@bobzhang bobzhang deleted the hongbo/tweak_logger_interface branch March 28, 2025 20:13
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.

2 participants