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

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

Merged
merged 10 commits into from
Jul 22, 2024

Conversation

RayStick
Copy link
Member

@RayStick RayStick commented Jul 9, 2024

Closes #63

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

Proposed Changes

  • (MINOR) dates are saved with less precision (2024-07-05_16-07-38.599493 --> 2024-07-05_16-07-38)
  • (MINOR) cli_alert_success replaced with cli_alert_info for more consistent console output
  • a new input to domain_mapping has been added (table_copy) - see the description of this argument in the function itself and the explanation in the README of what table copying means (particularly lines 348 onwards)
  • updated domain_mapping docs in general, to reflect some recent 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:

  • Please comment on my PR while it's a draft and give me feedback on the development!
  • I added everything I wanted to add to this PR, please review!
  • The title of this PR is clear and self-explantory.
  • I added any appropriate labels to this PR.

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Jul 9, 2024
@RayStick RayStick requested a review from Rainiefantasy July 9, 2024 17:16
@RayStick RayStick changed the title Table dependencies Table dependencies (quick for user and more valid) Jul 9, 2024
@RayStick RayStick changed the title Table dependencies (quick for user and more valid) Table dependencies (quicker for user and more valid) Jul 9, 2024
@RayStick RayStick added the enhancement Feature improvement or addition label Jul 9, 2024
@RayStick RayStick changed the title Table dependencies (quicker for user and more valid) Add in table dependencies (quicker for user and more valid) Jul 9, 2024
@RayStick RayStick marked this pull request as ready for review July 11, 2024 09:44
@RayStick RayStick requested review from BatoolMM and DDelbarre July 11, 2024 09:45
@RayStick
Copy link
Member Author

RayStick commented Jul 11, 2024

@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 🚀


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 RayStick marked this pull request as draft July 18, 2024 10:43
@RayStick
Copy link
Member Author

@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)
    ✅ ✅ works for both of us
  2. does table copying work with default output_dir and user specified output_dir
    ✅ ❌ 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)
    ✅ ✅ works for both of us

@RayStick RayStick marked this pull request as ready for review July 22, 2024 09:28
@Rainiefantasy
Copy link
Contributor

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

Copy link
Contributor

@Rainiefantasy Rainiefantasy left a comment

Choose a reason for hiding this comment

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

Approving for merge as tested changes and all looks good!

@RayStick RayStick merged commit 1adf8e7 into main Jul 22, 2024
2 checks passed
@RayStick
Copy link
Member Author

@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

@RayStick RayStick deleted the table-dependencies branch July 22, 2024 09:41
@BatoolMM
Copy link
Member

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
Copy link
Member Author

Amazing - thanks Batool!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement Feature improvement or addition
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Links between tables & variables
3 participants