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

readme for developers #476

Open
clarkliming opened this issue Apr 3, 2023 · 0 comments
Open

readme for developers #476

clarkliming opened this issue Apr 3, 2023 · 0 comments

Comments

@clarkliming
Copy link
Contributor

clarkliming commented Apr 3, 2023

to make chevron executable in scripts with arguments, we need to make sure

  1. the argument type should be one of the following types: logical, character, numeric, or (named nested) list of them
  2. vectors should not be named. list must be named. vector or list should not contain mixed data type
  3. match.arg can be used now.
  4. all pre functions should use :: to explicitly call functions, unless it is already called through "library" in script
  5. if reformatting is needed, reuse standard rules/create standard rule in dunlin (currently in chevron we create standard rules).
  6. In preprocessing, make assumptions on data input loose! there can be uncleaned data. factors/characters are possible
  7. arguments should not be too much!
  8. pre function should have very straightforward derivations for users to understand.
    1. ultimate responsibility goes to the end user for pre functions
    2. pre function, should contain basic derivations using dplyr/base R
    3. reformatting should be explicit (in mutate calls, use reformat(xxx, xxx), instead of using reformat.list method
    4. labels and variable derivation best in one place
    5. basic filtering anl01fl = "Y" can be added here
    6. filtering for PARAMCD, depends on if we would like to use it as a suffix.
  9. main function do use the variable derived from pre
    1. make it an argument if it can be modified by user
    2. (not sure) do not make it an argument if it should not be modified by user (e.g., EVENT = CNSR != 1)
    3. labels, use the variable labels, instead of creating a new argument(too much argument makes it difficult to read!)
  10. Add README.md!
  11. Add unit tests!
    3. unit tests should use cat(export_as_txt(...)) to make sure the output is good
  12. Assumptions on data
    5. adsl, under no circumstances, can be empty (there will not be outputs filter adsl to empty!)
    6. other datasets, could be empty
  13. avoid trim_rows as much as possible!
    2. it created table possibly not paginatable
  14. page_var need to be considered!
  15. whether to split by paramcd need to be considered!

Release related:

  1. tag on feature branch, not main
  2. submit package to autovalidator through hash or tag
  3. merge main after check passes
@clarkliming clarkliming pinned this issue Apr 3, 2023
@Teninq Teninq unpinned this issue Apr 7, 2023
@Teninq Teninq pinned this issue Apr 7, 2023
clarkliming added a commit that referenced this issue Apr 26, 2023
export checks and use `::` in pre functions

part of #476

---------

Signed-off-by: Liming <[email protected]>
Co-authored-by: b_falquet <[email protected]>
clarkliming added a commit that referenced this issue May 17, 2023
following #476, I tried to make the templates a bit clearer. something
added

1. for aet01_aesi and aet01 template, remove the check functions in pre.
all these derivations are executed without check! as users are
responsible for these derivations and they are clear. more checks added
in `main` functions
    1. check the arm variable are identical in levels (in adsl and adae)
    2. check variables to analyze exists (especially those new derived)
3. remove `deco`. titles/footnotes should be handled in citril using
lopo
1. remove aet01_2. this can be handled through arguments of aet01
1. for aet02 template, variants are removed. can be achieved through
argument `row_split_var`
2. add many utility functions (currently put in aet02.R but should go
into utility.R)
    3. valid_row_path. this checks if a row_path is valid for a table
4. valid_sort_at_path. this conduct the sort based on `row_path` if it
is valid. (sort_at_path gives error on incorrect row_path provided, and
aet02_1 previously will run into issues if prune_0 = FALSE on empty
data)
5. get_sort_path. this function will create "*" between variables, e.g,
`get_sort_path(c("A","B"))` will give "A" "*" "B", this is required by
`sort_at_path`
6. tlg_sort_by_var. this functions will sort using "A" and "B", instead
of "A" "*" "B". Also will check if the table contains the path. if does
not contain the path, the sorting will be ignored.

I think some of the funcitonality should go into tern/rtables. But for
now let's use them.

Please review this update in general and I will finalize this PR if no
objections. Then everyone can collaborate to harmonize all templates.

@Teninq @BFalquet @duanx9 @crazycatandy @barnett11

---------

Signed-off-by: b_falquet <[email protected]>
Co-authored-by: benoit <[email protected]>
Co-authored-by: b_falquet <[email protected]>
@BFalquet BFalquet unpinned this issue Jul 10, 2023
@clarkliming clarkliming pinned this issue Jul 27, 2023
@BFalquet BFalquet unpinned this issue Nov 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant