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

Site examples not using xportr_df_label() #179

Closed
rossfarrugia opened this issue Aug 17, 2023 · 9 comments · Fixed by #185
Closed

Site examples not using xportr_df_label() #179

rossfarrugia opened this issue Aug 17, 2023 · 9 comments · Fixed by #185
Assignees

Comments

@rossfarrugia
Copy link

@bms63 @elimillera i'm wondering why in all the site examples we show xportr_write("adsl.xpt", label = "Subject-Level Analysis Dataset"), when the xportr_df_label() function exists to write the dataset label out?

Wouldn't these examples be cleaner using the dataset labels direct from metacore$ds_spec rather than needing the user to manually provide these?

@rossfarrugia
Copy link
Author

Might be linked with this issue: #180

@EeethB EeethB assigned EeethB and vedhav and unassigned EeethB Nov 9, 2023
@EeethB
Copy link
Collaborator

EeethB commented Nov 9, 2023

Sounds like xportr_df_label() and xportr_write(label = ) do the same thing, but maybe in different ways?

@rossfarrugia
Copy link
Author

Sounds like xportr_df_label() and xportr_write(label = ) do the same thing, but maybe in different ways?

Seems one needs manual entry from the user (xportr_write()) and the other (xportr_df_label()) uses the metadata directly from the metacore object. Obviously the latter is preferable to reduce the chance of manual error.

@bms63
Copy link
Collaborator

bms63 commented Nov 14, 2023

@rossfarrugia We are brainstorming on having xportr_write() take in information from the metatdata for the dataset label. This way the xportr_df_label() function can be dropped from the workflow and take out the manual step from xportr_write().

image

@rossfarrugia
Copy link
Author

@bms63 sorry to add a spanner in works but we would still also have the use case to keep using xportr_df_label() on our side - reason being we write out as a parquet file first with the attributes added and then only create the xpt using xportr_write() separately at the end specifically for eSub purpose. So having the DF label separate to the XPT creation function is useful.

@rossfarrugia
Copy link
Author

I do agree with the idea of updating xportr_write() to use the metacore object metadata directly though 👍

@bms63
Copy link
Collaborator

bms63 commented Nov 14, 2023

I okay so we will keep both then @vedhav something to note

@vedhav
Copy link
Collaborator

vedhav commented Nov 16, 2023

Thank you for the insightful discussion, It was helpful in understanding the following problems:

  1. We have two implementations of setting the dataset label. One in xportr_df_label and another in xportr_write
  2. The dataset label is set in the xportr_df_label using metadata which is standard for xportr while we require a non-standard way of setting dataset label using the label argument in the xportr_write
  3. Right now, setting the label using xportr_df_label does retain the dataset label state, but it's not clear for the end user because of using a different implementation.

Solution as proposed in this thread:
Use the standard metadata argument in the xportr_write to stay consistent with the other xportr_* functions and depreciate the label argument. Make sure that the dataset label can be set using xportr_df_label or by passing the metadata.

@rossfarrugia
Copy link
Author

sounds a plan! and reminder that this issue is probably the most pressing here as its a bug: #180

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 a pull request may close this issue.

4 participants