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

Enhancement/tlg 1st listing function #181

Merged
merged 28 commits into from
Feb 17, 2025

Conversation

Gero1999
Copy link
Collaborator

@Gero1999 Gero1999 commented Jan 28, 2025

Issue

Closes #179

Description

Following the same strategy as with pkcg01 the aim is to create a function that can be used for the initial setup of TLGs > Listings.

Definition of Done

It has to have all the inputs that are also expected to be used from the App inputs:

  • Text input: Title: Listing of PK Concentration/n!PARAM: $PARAM /n !PCSPEC: $PCSPEC, !ROUTE: $ROUTE {by default = PARAM PCSPEC and ROUTE coming from Grouping tables variables}

  • Footnote input (by default= "*: Patients excluded from the summary table and mean plots")

  • [ ]Grouping tables: by default = c(“PARAM”, “PCSPEC”, “ROUTE”)

  • Grouping variables: SelectInput multiple choice (index columns) by default(c("TRT01A" or “TRTA”, "USUBJID", "AVISIT"))

  • Reported variables: Select input with relevant column names (default: NFRLT AFRLT, AVALC) (by default NFRLT AFRLT and AVALC)

  • Formatting table: 3 columns:

  • Variable name: Defined with Reported variables select input. No modifiable.

  • Label: Text input default populated with $Variable

  • Decimals: Select input with 6 options (1/2/3 decimals, 1/2/3 significant digits)

How to test

How to test features not covered by unit tests.

Contributor checklist

  • Code passes lintr checks
  • Code passes all unit tests
  • New logic covered by unit tests
  • New logic is documented
  • Package version is incremented

Notes to reviewer

Regarding the formatting table (last input defined) I changed it a little bit:

formatting table, columns

  • Variable name
  • Label
  • na_str
  • cero_str
  • align
  • format (only accepts official formats: formatters::list_valid_format_labels())

@Shaakon35 Keep in mind that even if any of these functions specifications we don't want to include them as shiny inputs they can still be kept as default and work within the function!

Anything that the reviewer should know before tacking the pull request?

Sorry, something went wrong.

@Gero1999 Gero1999 requested a review from Shaakon35 January 28, 2025 16:34
@Gero1999 Gero1999 linked an issue Jan 28, 2025 that may be closed by this pull request
9 tasks
@Gero1999
Copy link
Collaborator Author

Hey hey so this is the function that I was thinking in using to start with #186. Let me know what you think so far of it or if there is something missing/redundant @m-kolomanski @Shaakon35

@Gero1999
Copy link
Collaborator Author

Hey hey so this is the function that I was thinking in using to start with #186. Let me know what you think so far of it or if there is something missing/redundant @m-kolomanski @Shaakon35

I am personally thinking in substituting the formatting_vars_table column unit with a parsing action in Label of the kind of !AVAL ($AVALU). I think there might be an easy work-around, and the less columns we have in this table the better probably!

@m-kolomanski
Copy link
Collaborator

Hey, from my side, I do not have any thoughts at the moment. To be honest I do not have much experience with listings and I do not know what is the exact vision here (apart from being able to display -> customize -> export within the app). I can get down to code quality, but since this is just a draft in the works, I do not think it is the appropriate time.

Copy link
Collaborator

@Shaakon35 Shaakon35 left a comment

Choose a reason for hiding this comment

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

All good :)

@Gero1999 Gero1999 marked this pull request as ready for review January 31, 2025 12:59
@Gero1999
Copy link
Collaborator Author

Hello @m-kolomanski! The logic of the listings will be similar to the graphs of TLGs, only thing will change is the set of inputs (defined in #186) and that the output is not ggplot/ggplotly but rlistings.

Feel free to just review the code, but if possible also share your technical perspective on the bad aspects that the function has to be incorporated in the app. That way I can take your feedback into consideration while making the functions for the other type of TLGs left (Tables)

@Gero1999 Gero1999 requested a review from m-kolomanski February 3, 2025 08:09
Copy link
Collaborator

@m-kolomanski m-kolomanski left a comment

Choose a reason for hiding this comment

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

Looks nice, thank you for the changes!

Would be great if you bumped the package version before merging.

@Gero1999 Gero1999 merged commit 9cc800e into main Feb 17, 2025
10 checks passed
@Gero1999 Gero1999 deleted the enhancement/tlg-1st-listing-function branch March 6, 2025 09:49
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.

Enhancement: Create a listing function for the new setup
3 participants