aim-rsf / browseMetadata

An R package to help a researcher browse metadata for health datasets and categorise variables based on research domains
https://aim-rsf.github.io/browseMetadata/
GNU General Public License v3.0
3 stars 1 forks source link

Add in table dependencies (quicker for user and more valid) #105

Closed RayStick closed 1 week ago

RayStick commented 3 weeks ago

Closes #63

Copying from one table to the next will save the user time, and ensure consistency of categorisations across tables

Proposed Changes

In order to test the code you will need to process multiple tables

This is an example of how the copying should work (ignore the actual domain code values):

Screenshot 2024-07-17 at 12 05 38

Checklist before review:

RayStick commented 3 weeks ago

@BatoolMM @Rainiefantasy @DDelbarre - it would be great to get at least 2 of you reviewing this PR if that's possible, as it's a relatively major change for a user interaction, so I want to double check all is working fine and it cannot be broken in a way that I have not anticipated. After this PR is merged, I intend to make a new package release.

Thanks in advance :rocket:


Just to note: there have been a fair few changes to the code lately based on @DDelbarre's suggestions and @Rainiefantasy's review of these PRs. Main ones here:

RayStick commented 1 week ago

@Rainiefantasy thanks for the helpful testing so far 🚀

  1. does table copying work both as 1 run (selecting 2 tables at once) and 2 runs (each table has separate run) :white_check_mark: :white_check_mark: works for both of us
  2. does table copying work with default output_dir and user specified output_dir :white_check_mark: :x: only works for me - we need to investigate further
  3. does it work when output_dir is relative as well as absolute (and with or without / at end) :white_check_mark: :white_check_mark: works for both of us
Rainiefantasy commented 1 week ago

FYI seems like all is working - point 2 was erroring before but works fine now. I.e. all changes are working as expected 🤞

RayStick commented 1 week ago

@BatoolMM and @DDelbarre I am merging for now, as Mahwish & I did a fair bit of testing and troubleshooting. Would still value any user testing you have time for in the future

BatoolMM commented 1 week ago

Thank you @Rainiefantasy @RayStick - thsi looks wonderful - I did a very quick test and it seems to be working well. I have blocked time tomorrow morning to review more.

RayStick commented 1 week ago

Amazing - thanks Batool!