feat: make logger injectable in ResponsiveTable class #628
Conversation
- Add optional logger parameter to TableOptions type - Update ResponsiveTable constructor to accept injected logger function - Replace hardcoded logger.warn call with injected logger instance - Default to logger.warn when no custom logger is provided - Update test to use mock logger function instead of spying on global logger This change improves testability by allowing tests to inject custom loggers and increases flexibility by enabling different logging behaviors per table instance. The change is backward compatible as the logger parameter is optional and defaults to the existing behavior. Resolves dependency injection concerns and enhances unit test isolation by removing reliance on global logger state in tests.
|
Caution Review failedThe pull request is closed. WalkthroughAdds an optional logger function to TableOptions and wires ResponsiveTable to use it for warning about missing compact headers, defaulting to the existing logger.warn. Warning emission in getCompactIndices now calls the instance’s logger. Public API updated to include logger?: (message: string) => void. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant RT as ResponsiveTable
participant Log as Logger (injected or default)
Client->>RT: new ResponsiveTable({ logger?: fn })
Note right of RT: Store this.logger = options.logger || defaultWarn
Client->>RT: getCompactIndices(headers)
RT->>RT: Validate compact headers
alt Missing compact header
RT->>Log: warn("Compact header not found: ...")
Note over RT,Log: New: uses injected logger via this.logger
else All headers exist
RT-->>Client: indices
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @ryoppippi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request refactors the ResponsiveTable class to support logger injection, significantly improving its testability and flexibility. By allowing an external logger to be provided, the class no longer relies on global logging mechanisms, leading to better test isolation and more predictable behavior. The changes are fully backward compatible, ensuring existing implementations remain unaffected.
Highlights
- Logger Injection: Introduced an optional
loggerparameter to theTableOptionstype, allowing external logger functions to be injected into theResponsiveTableclass. - Improved Testability: The
ResponsiveTableconstructor now accepts an injected logger, defaulting tologger.warnif none is provided, which enhances testability by allowing mock loggers. - Code Refactoring: Replaced direct calls to
logger.warnwithin theResponsiveTableclass with the injectedthis.loggerinstance, centralizing logging behavior. - Enhanced Test Coverage: Updated existing tests to utilize a mock logger function for dependency injection, removing reliance on global state and improving test isolation.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
commit: |
There was a problem hiding this comment.
Code Review
This pull request introduces dependency injection for the logger in the ResponsiveTable class, which is a great improvement for testability and flexibility. The changes are well-implemented and maintain backward compatibility. I have a couple of suggestions to make the new logger type even more flexible by allowing for multiple arguments, which aligns better with common logging libraries.
| compactColAligns?: TableCellAlign[]; | ||
| compactThreshold?: number; | ||
| forceCompact?: boolean; | ||
| logger?: (message: string) => void; |
There was a problem hiding this comment.
The logger type (message: string) => void is a bit restrictive as it only allows a single string argument. Common logging functions, including consola which is used in this project, often accept multiple arguments or structured data (e.g., logger.warn('message', { details: '...' })). To improve flexibility and align better with standard logging practices, consider using a more generic type like (...args: any[]) => void.
| logger?: (message: string) => void; | |
| logger?: (...args: any[]) => void; |
| private compactThreshold: number; | ||
| private compactMode = false; | ||
| private forceCompact: boolean; | ||
| private logger: (message: string) => void; |
There was a problem hiding this comment.
For consistency with the recommended change in TableOptions, this property's type should also be updated to (...args: any[]) => void to allow for more flexible logging.
| private logger: (message: string) => void; | |
| private logger: (...args: any[]) => void; |
Summary
This PR introduces logger injection capability to the
ResponsiveTableclass, improving testability and flexibility while maintaining backward compatibility.What Changed
TableOptionstype with signature(message: string) => voidlogger.warnWhy
Testing
Related Issues
This addresses dependency injection concerns and improves the overall architecture of the ResponsiveTable class by following better software engineering practices.
Summary by CodeRabbit