-
-
Notifications
You must be signed in to change notification settings - Fork 672
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
refactor: set correct log level #478
Conversation
Signed-off-by: rare-magma <[email protected]>
Signed-off-by: rare-magma <[email protected]>
WalkthroughThe changes in this pull request involve modifications to the logging mechanism in the Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/app-gocardless/banks/integration-bank.js (4)
31-34
: Approved: Logging level adjustment implemented correctly.The change from
console.log
toconsole.debug
aligns with the PR objective of adjusting the logging level for bank integration messages. This is a good practice for debugging information that shouldn't appear in production logs.Consider using a more descriptive message for the debug log:
console.debug( 'Normalizing account properties for new institution integration', { account: JSON.stringify(account) }, );This minor change would make the purpose of the log more immediately clear to developers reviewing the logs.
69-73
: Approved: Logging level adjustment implemented correctly.The change from
console.log
toconsole.debug
is consistent with the PR objective and previous changes. Good practice to log only the first 10 transactions to avoid overwhelming the logs.For consistency with the previous log message, consider updating the debug message:
console.debug( 'Sorting transactions for new institution integration', { top10Transactions: JSON.stringify(transactions.slice(0, 10)) }, );This change maintains a consistent style across debug messages in the file.
Line range hint
77-86
: Approved: Logging level adjustment implemented correctly.The change from
console.log
toconsole.debug
is consistent with the PR objective and previous changes. Good practice to log balances and only the first 10 transactions to avoid overwhelming the logs.For consistency and clarity, consider updating the debug message:
console.debug( 'Calculating starting balance for new institution integration', { balances: JSON.stringify(balances), top10SortedTransactions: JSON.stringify( sortedTransactions.slice(0, 10), ), }, );This change maintains a consistent style across debug messages in the file and clearly indicates the purpose of the function.
Line range hint
1-101
: Summary: Logging level adjustments successfully implemented.The changes in this file consistently replace
console.log
withconsole.debug
for debugging information related to account normalization, transaction sorting, and starting balance calculation. These adjustments align well with the PR objective of setting the correct log level for bank integration messages.Key points:
- The core functionality of the integration logic remains unchanged.
- Debug logs now include relevant information for troubleshooting without affecting production logs.
- The changes improve the clarity and relevance of logged information as intended.
Consider implementing a centralized logging mechanism (if not already in place) that allows for easy toggling of debug logs in different environments. This would provide more flexibility in managing log levels across the application.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/478.md
is excluded by!**/*.md
📒 Files selected for processing (2)
- src/app-gocardless/banks/integration-bank.js (2 hunks)
- src/app-gocardless/banks/tests/integration-bank.spec.js (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
src/app-gocardless/banks/tests/integration-bank.spec.js (3)
12-12
: LGTM: Consistent logging level adjustmentThe change from
console.log
toconsole.debug
in the test setup is consistent with the PR objective and the changes made in the main code. This ensures that the tests accurately reflect the behavior of the actual implementation.
Line range hint
1-165
: Summary of changesThe changes in this file align with the PR objective of adjusting the logging level from
info
todebug
. The test setup has been updated to mockconsole.debug
instead ofconsole.log
, which is consistent with the changes made in the main code.Key points:
- The core functionality of the tests remains unchanged.
- The logging level adjustment improves consistency between the implementation and tests.
- We've requested verification for potential additional changes mentioned in the AI summary but not visible in the provided code snippet.
Overall, these changes contribute to better logging practices and maintain the integrity of the test suite.
Line range hint
1-165
: Verify completeness of changesThe AI summary mentions changes in other test cases (e.g.,
normalizeAccount
,sortTransactions
, andcalculateStartingBalance
), but these changes are not visible in the provided code snippet. To ensure a comprehensive review:
- Please confirm if there are additional changes in this file that need to be reviewed.
- If there are missing changes, kindly provide the complete diff or updated file content.
To help identify any missing changes, you can run the following command:
If you need assistance reviewing any additional changes or if you'd like me to generate updated test cases for the modified logging behavior, please let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this!
This PR will set the correct log level for bank integration messages which previously were logging unnecessary details at info level.