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.

Sign up for GitHub

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

Jump to bottom

Reorganize and group defaults for stats and their default formats/labels -> addition of auto for DMT01 #1051

Merged
merged 77 commits into from
Sep 11, 2023
Merged

Reorganize and group defaults for stats and their default formats/labels -> addition of auto for DMT01 #1051

merged 77 commits into from
Sep 11, 2023

Conversation

Melkiades
Copy link
Contributor

@Melkiades Melkiades commented Aug 31, 2023 •
edited
Loading

Fixes #994 (for now)

Still todo:

  • tests for filtering
  • Examples
  • Evaluate the effect of having a list instead of a character vector (it was the second, I am more inclined towards first)
  • start deprecation of summary_formats, summary_labels
  • discuss: consider changes in summary_custom (to discuss with @edelarua)
  • wontdo: replace it within count_occurences and summarize_num_patients (once reworked, wont do for now)
  • wontdo: consider using automatic function name getter for calling function (related to previous. Not necessary until reworked)
  • wontdo: start deprecating other defaults (not related for now)

@Melkiades Melkiades mentioned this pull request Aug 31, 2023
Closed
@Melkiades Melkiades marked this pull request as ready for review September 5, 2023 11:08
@Melkiades Melkiades marked this pull request as draft September 5, 2023 11:24
edelarua
edelarua requested changes Sep 7, 2023
View reviewed changes
Copy link
Contributor

@edelarua edelarua left a comment

Choose a reason for hiding this comment

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

Looks great - everything is working for me! Just a few wording suggestions, and you should also rename the test file to match the renamed R file and then rerun tests.

R/argument_convention.R Outdated Show resolved Hide resolved
R/utils_factor.R Outdated Show resolved Hide resolved
Melkiades and others added 10 commits September 8, 2023 09:39
@Melkiades
renaming
26cb616
@Melkiades @edelarua
Apply suggestions from code review
732d9d5 
Co-authored-by: Emily de la Rua <59304861+edelarua@users.noreply.github.com>
Signed-off-by: Davide Garolini <dgarolini@gmail.com>
@dependabot-preview
[skip actions] Roxygen Man Pages Auto Update
053f518
@Melkiades
Merge branch '994_reorganize_defaults@main' of github.com:insightseng…
edd3e3a 
…ineering/tern into 994_reorganize_defaults@main
@Melkiades
fixed pkg_down
45139af
@Melkiades
lint fixes
a23e0ac
@Melkiades
Merge branch 'main' into 994_reorganize_defaults@main
8e47119 
Signed-off-by: Davide Garolini <davide.garolini@roche.com>
@Melkiades
Merge branch 'main' into 994_reorganize_defaults@main
8a5e168 
Signed-off-by: Davide Garolini <davide.garolini@roche.com>
@Melkiades
lintfix
25486e5
@Melkiades
Merge branch '994_reorganize_defaults@main' of github.com:insightseng…
23763c4 
…ineering/tern into 994_reorganize_defaults@main
@Melkiades
Copy link
Contributor Author

@edelarua is it good to go? :)

edelarua
edelarua approved these changes Sep 8, 2023
View reviewed changes
Copy link
Contributor

@edelarua edelarua left a comment

Choose a reason for hiding this comment

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

Good to go! Thanks Davide :)

@Melkiades
Merge branch 'main' into 994_reorganize_defaults@main
a6ea43f 
Signed-off-by: Davide Garolini <davide.garolini@roche.com>
@Melkiades Melkiades enabled auto-merge (squash) September 11, 2023 16:26
@Melkiades Melkiades merged commit ec8c413 into main Sep 11, 2023
@Melkiades Melkiades deleted the 994_reorganize_defaults@main branch September 11, 2023 18:49
@edelarua edelarua mentioned this pull request Oct 26, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BFalquet BFalquet BFalquet left review comments

@pawelru pawelru pawelru left review comments

@edelarua edelarua edelarua approved these changes

Assignees
No one assigned
Labels
enhancement sme
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cautiously integrate/introduce expanded and more elastic standards for summaries' formats
4 participants
@Melkiades @pawelru @edelarua @BFalquet