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

# 16 calculate_study_day #20

Merged
merged 75 commits into from
Mar 13, 2024
Merged

# 16 calculate_study_day #20

merged 75 commits into from
Mar 13, 2024

Conversation

yli110-stat697
Copy link
Collaborator

close #16

@yli110-stat697 yli110-stat697 marked this pull request as draft November 6, 2023 16:08
Copy link

github-actions bot commented Nov 6, 2023

Code Coverage

Package Line Rate Health
sdtm.oak 86%
Summary 86% (349 / 408)

@yli110-stat697
Copy link
Collaborator Author

Hi @ShiyuC can you take a look at this design and see if it's okay? If so, I can start to add documentation and unit tests

@ShiyuC
Copy link
Collaborator

ShiyuC commented Nov 28, 2023

Hi @ShiyuC can you take a look at this design and see if it's okay? If so, I can start to add documentation and unit tests

@yli110-stat697 Hi Rosemary, thanks for creating this PR. It looks good to me. Feel free to move on with documentation and unit test at your convenience

@yli110-stat697 yli110-stat697 marked this pull request as ready for review December 6, 2023 20:06
@ShiyuC
Copy link
Collaborator

ShiyuC commented Dec 8, 2023

Hi @yli110-stat697! Could you create a test within the testthat folder so that we could run the function with your test? Thanks!

@yli110-stat697
Copy link
Collaborator Author

Hi @yli110-stat697! Could you create a test within the testthat folder so that we could run the function with your test? Thanks!

Hi yes I can, do we agree on the design already?

R/calculate_study_day.R Outdated Show resolved Hide resolved
R/calculate_study_day.R Outdated Show resolved Hide resolved
R/calculate_study_day.R Outdated Show resolved Hide resolved
R/calculate_study_day.R Outdated Show resolved Hide resolved
R/calculate_study_day.R Outdated Show resolved Hide resolved
@yli110-stat697 yli110-stat697 self-assigned this Jan 20, 2024
@ramiromagno
Copy link
Collaborator

EDIT, EDIT: Now that I've check how study day is calculated, I think it is actually simpler to implement. Please take a look at my commit.

Thanks @ramiromagno I think it looks good except that the condition should have equal sign. BTW, you can use "Suggest" functionality instead of pushing commit to show your suggestion. Developer can directly commit your "suggestion". Because forgetting to pull other people's commit usually causes merge conflict locally.

Screenshot 2024-02-19 at 9 55 35 AM

Hi @yli110-stat697 : Thanks for the tip. Will do next time (sorry for the inconvenience!).

@yli110-stat697
Copy link
Collaborator Author

Hi @galachad my pipeline failed due to pandoc installation error. I belive this has nothing to do with my code. How do I fix this?

@ramiromagno ramiromagno self-requested a review February 22, 2024 18:24
R/calculate_study_day.R Outdated Show resolved Hide resolved
R/calculate_study_day.R Outdated Show resolved Hide resolved
R/calculate_study_day.R Outdated Show resolved Hide resolved
R/calculate_study_day.R Outdated Show resolved Hide resolved
R/calculate_study_day.R Outdated Show resolved Hide resolved
R/calculate_study_day.R Outdated Show resolved Hide resolved
R/calculate_study_day.R Outdated Show resolved Hide resolved
@ramiromagno ramiromagno self-requested a review March 2, 2024 22:43
@edgar-manukyan edgar-manukyan removed their request for review March 4, 2024 15:04
Copy link
Collaborator

@ShiyuC ShiyuC left a comment

Choose a reason for hiding this comment

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

The MR looks good to me. Just commented on one small typo. Thanks @yli110-stat697 for all of your effort!

R/derive_study_day.R Outdated Show resolved Hide resolved
yli110-stat697 and others added 2 commits March 11, 2024 11:09
Copy link
Collaborator

@ramiromagno ramiromagno left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@rammprasad rammprasad merged commit 375b11f into main Mar 13, 2024
15 checks passed
@rammprasad rammprasad deleted the 16_calculate_study_day@devel branch March 13, 2024 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Develop a function to calculate study day
5 participants