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

Grouped report_participants() does not report gender unless argument gender is capitalized #378

Merged
merged 5 commits into from
Jul 1, 2023

Conversation

strengejacke
Copy link
Member

Fixes #377

@codecov
Copy link

codecov bot commented Jun 27, 2023

Codecov Report

Merging #378 (ca3f50b) into main (75ae425) will increase coverage by 0.61%.
The diff coverage is 100.00%.

❗ Current head ca3f50b differs from pull request most recent head 2f2273c. Consider uploading reports for the commit 2f2273c to get more accurate results

@@            Coverage Diff             @@
##             main     #378      +/-   ##
==========================================
+ Coverage   71.62%   72.23%   +0.61%     
==========================================
  Files          47       47              
  Lines        3313     3314       +1     
==========================================
+ Hits         2373     2394      +21     
+ Misses        940      920      -20     
Impacted Files Coverage Δ
R/report_participants.R 92.77% <100.00%> (+7.27%) ⬆️

@strengejacke
Copy link
Member Author

I can't merge, commits are not signed... 🤔

@rempsyc
Copy link
Sponsor Member

rempsyc commented Jun 27, 2023

I think there is just the lints. But I can take care of it this weekend if that's ok with you

@strengejacke
Copy link
Member Author

Strange I cannot sqash'n'merge... ?

@rempsyc
Copy link
Sponsor Member

rempsyc commented Jun 27, 2023

It is because lint workflow has to pass to allow merging

@strengejacke
Copy link
Member Author

strengejacke commented Jun 27, 2023

Yeah, but why? At least there could be an option to skip a requirement, which we have in other repos...

@rempsyc
Copy link
Sponsor Member

rempsyc commented Jun 27, 2023

It is to make sure that all tests pass before merging to save time/work in the future. I would prefer that we don't merge when checks don't pass.

@strengejacke
Copy link
Member Author

Given the error proneness of certain tests or that error messages are hardly avoidable in certain tests, I think that's s very strict policy 😉

@rempsyc
Copy link
Sponsor Member

rempsyc commented Jul 1, 2023

Fair enough. But I think the general rule should be to try to address the lints/failing errors, and request an exception if really it is a capricious situation. I'm afraid that if it is possible to easily skip them, we end up always skipping them even when they are avoidable. In this particular case, it was lints that I felt I could address. I also volunteer to fix the lints check in all future PRs of report :P

@rempsyc rempsyc merged commit dd6e080 into main Jul 1, 2023
26 checks passed
@rempsyc rempsyc deleted the strengejacke/issue377 branch July 1, 2023 01:06
@rempsyc
Copy link
Sponsor Member

rempsyc commented Jul 1, 2023

I merged but we get several warnings like below in R CMD check:

 ── Warning ('test-report.lm.R:39:3'): report.lm - lm intercept-only ────────────
  Using `$` in model formulas can produce unexpected results. Specify your
    model using the `data` argument instead.

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.

Grouped report_participants() does not report gender unless argument gender is capitalized
2 participants