Fix log tag to use TAG constant #421
Conversation
Summary of ChangesHello @helpingstar, 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 introduces a minor refactoring to enhance code consistency by replacing a hardcoded log tag string with a predefined constant. This change specifically targets a log statement within the tap-to-focus functionality, ensuring that log tags adhere to established conventions. Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
| val surfaceCoords = it.transform() | ||
| Log.d( | ||
| "TAG", | ||
| TAG, |
There was a problem hiding this comment.
While the change to use the TAG constant is an improvement for consistency, the presence of this Log.d statement itself should be scrutinized 1. The repository's guidelines suggest that Log.d calls are often remnants of debugging and should be removed before merging unless they provide essential, long-term value. Please consider if this log statement is necessary for the long-term operation of the application.
Style Guide References
Footnotes
-
Scrutinize Debug Logs: Question the use of
Log.d,Log.v, and especiallyprintln(). These are often remnants of debugging and should be removed before merging unless they provide essential, long-term value. ↩
Summary of ChangesHello @helpingstar, 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 enhances code consistency and maintainability by standardizing the log tag used in a specific logging statement. By replacing a hardcoded string with a constant, it ensures that log messages originating from the tap-to-focus feature are uniformly tagged, making log filtering and debugging more reliable. Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 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
|
There was a problem hiding this comment.
Code Review
This pull request correctly replaces a hardcoded log tag with the TAG constant, which is a good improvement. I've also identified a related issue in the log message formatting and provided a suggestion to fix it. This will ensure the log output is correct and also makes the code more idiomatic.
| Log.d( | ||
| "TAG", | ||
| TAG, | ||
| "onTapToFocus: " + | ||
| "input{$it} -> surface{$surfaceCoords}" | ||
| ) |
There was a problem hiding this comment.
While you're fixing the log tag, there's an opportunity to improve the log message itself. The current string concatenation will not interpolate the variables it and surfaceCoords, which means the log will not contain the expected values. Using a Kotlin template string will fix this bug and make the code more concise and readable.1
Log.d(
TAG,
"onTapToFocus: input{$it} -> surface{$surfaceCoords}"
)Style Guide References
Footnotes
-
The style guide recommends simplifying complex logic into more concise and readable idiomatic expressions. Using a template string instead of string concatenation is a good example of this. ↩
temcguir
left a comment
temcguir
left a comment
There was a problem hiding this comment.
Thanks for the catch on this.
2 participants
Changed hardcoded "TAG" string to TAG constant in tap-to-focus log statement
jetpack-camera-app/ui/components/capture/src/main/java/com/google/jetpackcamera/ui/components/capture/CaptureScreenComponents.kt
Lines 468 to 475 in 6ba412b
Please note: I understand that minor typo fixes might not be the priority for this project. If you prefer not to accept this type of contribution, I'm happy to close this PR. No hard feelings!