Closes #151 - Metadata includes `verbose` option #199

EeethB · 2023-12-05T21:59:11Z

Thank you for your Pull Request!

We have developed a Pull Request template to aid you and our reviewers. Completing the below tasks helps to ensure our reviewers can maximize their time on your code as well as making sure the xportr codebase remains robust and consistent.

The scope of `{xportr}`

{xportr}'s scope is to enable R users to write out submission compliant xpt files that can be delivered to a Health Authority or to downstream validation software programs. We see labels, lengths, types, ordering and formats from a dataset specification object (SDTM and ADaM) as being our primary focus. We also see messaging and warnings to users around applying information from the specification file as a primary focus. Please make sure your Pull Request meets this scope of {xportr}. If your Pull Request moves beyond this scope, please get in touch with the {xportr} team on slack or create an issue to discuss.

Please check off each task box as an acknowledgment that you completed the task. This checklist is part of the Github Action workflows and the Pull Request will not be merged into the devel branch until you have checked off each task.

Changes Description

Adds a verbose option to xportr_metadata()
Defaults verbose to NULL in all other functions
Chooses verbose value in this priority order:
- Explicit value set in function call
- Value set in earlier call to xportr_metadata()
- Option value: _xportr.[type|length|order|label]_verbose_

Task List

codecov · 2023-12-06T22:34:43Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c0dd2f0) 100.00% compared to head (9d91212) 100.00%.
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #199   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           13        13           
  Lines          612       627   +15     
=========================================
+ Hits           612       627   +15

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bms63

Is it possible to create a test-metadata.R file and move al the metadata related tests into that file. Just to help keep things organized and findable

NEWS.md

Co-authored-by: Ben Straub <[email protected]>

EeethB · 2024-01-04T16:37:15Z

Is it possible to create a test-metadata.R file and move al the metadata related tests into that file. Just to help keep things organized and findable

I made this change, let me know what you think!

bms63 · 2024-01-16T16:28:10Z

Hey - @EeethB when you get a chance can you fixt these merge conflicts please

EeethB · 2024-01-16T23:54:07Z

Hey - @EeethB when you get a chance can you fixt these merge conflicts please

@bms63 merge conflicts fixed

tests/testthat/test-metadata.R

averissimo

I'm also a bit concerned about the effect of calling xportr_metadata multiple times with different arguments.

It might overwrite previously set metadata / verbose (started a discussion on #224)

We can discuss and deal with it in that issue.

averissimo · 2024-01-23T10:53:06Z

R/metadata.R

+    `_xportr.df_metadata_` = metadata,
+    `_xportr.df_verbose_` = verbose


We could use quotes ", instead of backticks

Suggested change

`_xportr.df_metadata_` = metadata,

`_xportr.df_verbose_` = verbose

"_xportr.df_metadata_" = metadata,

"_xportr.df_verbose_" = verbose

averissimo · 2024-01-23T10:53:25Z

R/metadata.R

@@ -51,5 +54,8 @@ xportr_metadata <- function(.df, metadata = NULL, domain = NULL) {

  ## End of common section

-  structure(.df, `_xportr.df_metadata_` = metadata)
+  structure(.df,


Very very optional. I would think that {styler} or {lintr} didn't like this indentation

Suggested change

structure(.df,

structure(

.df,

bms63 · 2024-01-25T01:13:33Z

@EeethB Ping me after you fix these merge conflicts and I can take a look

* 226_fix_xportr_write: fix/revert: metadata is no longer inferred

averissimo · 2024-01-25T17:26:35Z

@EeethB I merged the main branch to correct the conflicts cause by #190 (trying to save you the trouble)

The CI started to fail and it was due to a bug
- see Bug: metadata should not be infered in xportr_write #226
- corrected with Closes #226 - metadata is no longer inferred on xportr_write #227

Additionally, the placement of the inferrement of the verbose from attributes was causing tests to fail and I moved it to the common section.

Sorry about this 😕.

EeethB · 2024-01-29T16:20:29Z

@averissimo thanks for doing all that! It looks like all the checks are passing now, @bms63 I think this is ready for one more review?

bms63

Many thank yous @EeethB and @averissimo for helping to see this PR/issue through!!

Merge remote-tracking branch 'origin/main' into 91-max-length # Conflicts: # R/length.R # man/metadata.Rd # man/xportr_length.Rd

EeethB force-pushed the 151_metadata_verbose branch from 0bb0ef8 to ab82633 Compare December 5, 2023 21:59

EeethB changed the title ~~151 metadata verbose~~ Closes #151 Dec 5, 2023

EeethB linked an issue Dec 5, 2023 that may be closed by this pull request

Feature Request: xportr_metadata() should allow passing of verbose as well #151

Closed

EeethB changed the title ~~Closes #151~~ Closes #151 - Metadata includes verbose option Dec 5, 2023

EeethB added 5 commits December 6, 2023 22:31

Add verbose option to metadata

3dcf6fb

Add verbose to metadata

26bbc8e

Add verbose option to metadata

f4a4b4d

Update docs

f534680

Update news and README

a0d83b7

EeethB force-pushed the 151_metadata_verbose branch from 0a582b1 to a0d83b7 Compare December 6, 2023 22:32

EeethB marked this pull request as ready for review December 6, 2023 22:44

EeethB requested a review from bms63 December 7, 2023 16:34

Merge branch 'main' into 151_metadata_verbose

d7da839

bms63 reviewed Dec 14, 2023

View reviewed changes

NEWS.md Outdated Show resolved Hide resolved

EeethB and others added 2 commits December 19, 2023 12:12

Update NEWS.md

ce6fc72

Co-authored-by: Ben Straub <[email protected]>

Move metadata tests all to a single file

3f79aaa

Merge branch 'main' into 151_metadata_verbose

4cc529f

averissimo mentioned this pull request Jan 16, 2024

Remove dynamic domain determination #193

Merged

14 tasks

EeethB added 2 commits January 16, 2024 17:27

Merge branch 'main' into 151_metadata_verbose

71390ef

Fix failing test by specifying domain

ef70444

EeethB requested a review from bms63 January 16, 2024 23:54

bms63 reviewed Jan 17, 2024

View reviewed changes

tests/testthat/test-metadata.R Outdated Show resolved Hide resolved

EeethB added 2 commits January 18, 2024 16:23

Add integration test for metadata

3f5d9e0

Merge remote-tracking branch 'origin/main' into 151_metadata_verbose

1e0939b

EeethB requested a review from bms63 January 19, 2024 22:09

averissimo reviewed Jan 23, 2024

View reviewed changes

averissimo added 3 commits January 25, 2024 17:15

Merge branch 'main' into 151_metadata_verbose

264d23b

Merge branch '226_fix_xportr_write' into 151_metadata_verbose

e1db193

* 226_fix_xportr_write: fix/revert: metadata is no longer inferred

fix: move verbose inferrence to common section

50aa324

averissimo mentioned this pull request Jan 25, 2024

Closes #137 - Add xportr_process() to apply all and write #200

Merged

14 tasks

bms63 added 2 commits January 29, 2024 13:18

Merge branch 'main' into 151_metadata_verbose

1fed719

Merge branch 'main' into 151_metadata_verbose

9d91212

bms63 approved these changes Jan 29, 2024

View reviewed changes

bms63 merged commit eb3428d into main Jan 29, 2024
13 checks passed

bms63 deleted the 151_metadata_verbose branch January 29, 2024 18:30

bms63 added a commit that referenced this pull request Jan 29, 2024

fix: #91 resolve merge conflicts from #199

4023016

Merge remote-tracking branch 'origin/main' into 91-max-length # Conflicts: # R/length.R # man/metadata.Rd # man/xportr_length.Rd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Closes #151 - Metadata includes `verbose` option #199

Closes #151 - Metadata includes `verbose` option #199

EeethB commented Dec 5, 2023 •

edited

Loading

codecov bot commented Dec 6, 2023 •

edited

Loading

bms63 left a comment

EeethB commented Jan 4, 2024

bms63 commented Jan 16, 2024

EeethB commented Jan 16, 2024

averissimo left a comment

averissimo Jan 23, 2024

averissimo Jan 23, 2024 •

edited

Loading

bms63 commented Jan 25, 2024

averissimo commented Jan 25, 2024

EeethB commented Jan 29, 2024

bms63 left a comment

		`_xportr.df_metadata_` = metadata,
		`_xportr.df_verbose_` = verbose

Closes #151 - Metadata includes verbose option #199

Closes #151 - Metadata includes verbose option #199

Conversation

EeethB commented Dec 5, 2023 • edited Loading

Thank you for your Pull Request!

The scope of {xportr}

Changes Description

Task List

codecov bot commented Dec 6, 2023 • edited Loading

Codecov Report

bms63 left a comment

Choose a reason for hiding this comment

EeethB commented Jan 4, 2024

bms63 commented Jan 16, 2024

EeethB commented Jan 16, 2024

averissimo left a comment

Choose a reason for hiding this comment

averissimo Jan 23, 2024

Choose a reason for hiding this comment

averissimo Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

bms63 commented Jan 25, 2024

averissimo commented Jan 25, 2024

EeethB commented Jan 29, 2024

bms63 left a comment

Choose a reason for hiding this comment

Closes #151 - Metadata includes `verbose` option #199

Closes #151 - Metadata includes `verbose` option #199

EeethB commented Dec 5, 2023 •

edited

Loading

The scope of `{xportr}`

codecov bot commented Dec 6, 2023 •

edited

Loading

averissimo Jan 23, 2024 •

edited

Loading