-
Notifications
You must be signed in to change notification settings - Fork 50
style(countMissingPercentage): Fix styling of arrows to equal signs #172
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
Conversation
WalkthroughReplaced R assignment operator Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
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
🧹 Nitpick comments (2)
R/utils_groupcomparison.R (1)
409-409: Style-only change looks good; consider making GROUP column reference explicit to avoid NSE pitfalls.The switch from
<-to=is fine and consistent with the surrounding file style. Minor robustness nit: referencingGROUPas a bare symbol outside a data.table j/by context relies on the current environment. Making the column reference explicit improves clarity and avoids accidental name masking.Apply this minimal, safer refactor:
- counts = counts[match(intersect(colnames(contrast_matrix), GROUP), GROUP), ] + idx = match(intersect(colnames(contrast_matrix), counts$GROUP), counts$GROUP) + counts = counts[idx, ]inst/tinytest/test_utils_groupcomparison.R (1)
83-84: Optional: add a test asserting column/order stability for repeated runs.You already validate ordering with permuted column names. Consider one more assertion that
output$MissingPercentagealigns withrownames(contrast_matrix)when there are repeated GROUP rows pre-aggregation, to guard against future refactors in the counting step.If useful, I can draft a small additional test case that duplicates GROUP rows before aggregation and checks the order and values.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
R/utils_groupcomparison.R(1 hunks)inst/tinytest/test_utils_groupcomparison.R(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
inst/tinytest/test_utils_groupcomparison.R (2)
3-6: LGTM on assignment operator normalization across tests.All changes are purely stylistic (
<-→=) and preserve test intent and calculations. The suite continues to exercise ordering, empty-group handling, and imputation logic appropriately.Also applies to: 12-13, 22-25, 35-42, 43-46, 52-54, 61-63, 68-72, 78-84, 102-104, 110-114, 117-131, 134-141
102-114: Edge-case assertion looks correct.The “complete missing group” case correctly computes 0.2. No issues introduced by the assignment style change.
User description
Motivation and Context
Accidentally used arrows instead of equal signs in #171
Changes
Testing
Existing unit tests pass
Checklist Before Requesting a Review
PR Type
Enhancement, Tests
Description
Standardize assignment operators to '='
Align function usage across code, tests
Minor refactor in counts indexing
Diagram Walkthrough
File Walkthrough
utils_groupcomparison.R
Use '=' for counts reassignmentR/utils_groupcomparison.R
test_utils_groupcomparison.R
Tests updated to '=' assignment styleinst/tinytest/test_utils_groupcomparison.R
Summary by CodeRabbit