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

Replace group with .group #102

Merged
merged 12 commits into from
Jul 7, 2022
Merged

Conversation

adrian-lison
Copy link
Collaborator

This PR should fix #99 on the develop branch. All group, old_group and new_group variables have been prefixed with a dot.

Same for old_group and new_group.
@adrian-lison adrian-lison requested a review from seabbs July 6, 2022 14:29
@adrian-lison adrian-lison added enhancement New feature or request high-priority labels Jul 6, 2022
@seabbs seabbs marked this pull request as ready for review July 6, 2022 14:34
@adrian-lison adrian-lison linked an issue Jul 6, 2022 that may be closed by this pull request
Copy link
Collaborator

@seabbs seabbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice stuff, it looks like you've got everything. The only thing that appears to be missing is that the script in inst that generates the examples needs updating (this is causing the scoring tests to fail at the moment (probably have some more tests using these examples as a way of checking we are reproducing past results as here obviously been helpful).

I quickly added a check function to look for these variables, restyled after merging develop, and added some tests for the new check function.

Once examples are updated and there is a news item for this PR very happy for this to be merged in.

@codecov
Copy link

codecov bot commented Jul 7, 2022

Codecov Report

Merging #102 (acfa8db) into develop (ba6f217) will increase coverage by 0.58%.
The diff coverage is 65.67%.

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

@@             Coverage Diff             @@
##           develop     #102      +/-   ##
===========================================
+ Coverage    62.57%   63.16%   +0.58%     
===========================================
  Files           12       12              
  Lines          946      961      +15     
===========================================
+ Hits           592      607      +15     
  Misses         354      354              
Impacted Files Coverage Δ
R/formula-tools.R 92.14% <ø> (ø)
R/model.R 33.52% <0.00%> (ø)
R/utils.R 65.71% <0.00%> (ø)
R/postprocess.R 38.14% <25.00%> (ø)
R/preprocess.R 94.68% <93.10%> (+0.05%) ⬆️
R/check.R 81.25% <100.00%> (+12.82%) ⬆️
R/model-design-tools.R 86.88% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ba6f217...3664315. Read the comment docs.

NEWS.md Outdated Show resolved Hide resolved
NEWS.md Show resolved Hide resolved
R/check.R Show resolved Hide resolved
@seabbs
Copy link
Collaborator

seabbs commented Jul 7, 2022

This all looks good to me. Happy to see it merged in when you are ready.

@adrian-lison
Copy link
Collaborator Author

To me as well. Here we go!

@adrian-lison adrian-lison merged commit 598f855 into develop Jul 7, 2022
@adrian-lison adrian-lison deleted the refactor_rename_group_variable branch July 7, 2022 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request high-priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename group variable to be more unique
2 participants