Skip to content

Migrate metadata from safetyGraphics to safetyCharts#650

Merged
jwildfire merged 20 commits intodevfrom
fix-637
Mar 23, 2022
Merged

Migrate metadata from safetyGraphics to safetyCharts#650
jwildfire merged 20 commits intodevfrom
fix-637

Conversation

@jwildfire
Copy link
Contributor

@jwildfire jwildfire commented Nov 4, 2021

Overview

This PR refactors safetyGraphics metadata workflow to allow users to easily add metadata for new charts without needing to update the safetyGraphics package.

Updates include:

Test Notes

Test in in conjunction with SafetyGraphics/safetyCharts#97

  • Code review of makeMeta.R
  • Confirm all tests in test_makeMeta.R make sense and run as expected.
  • Review updates Examples 11 and 12 from the safetyGraphics cookbook vignette and confirm that code runs as expected.
  • Review updates to the safetyGraphics chart vignette

Fixes #637

@jwildfire jwildfire self-assigned this Feb 28, 2022
@jwildfire jwildfire marked this pull request as draft February 28, 2022 15:11
@jwildfire jwildfire marked this pull request as ready for review February 28, 2022 15:57
Copy link
Contributor

@xni7 xni7 left a comment

Choose a reason for hiding this comment

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

@jwildfire , looks great! PR review notes

  • Code review of makeMeta.R
    • code looks good!
  • Confirm all tests in test_makeMeta.R make sense and run as expected.
    • All tests passed
  • Review updates Examples 11 and 12 from the safetyGraphics cookbook vignette and confirm that code runs as expected.
    • Example 11 works. Couldn't find Example 12
  • Review updates to the safetyGraphics chart vignette
    • Is chart vignette updated? don't see the new content

Had a minor comment about extending existing standard meta data.

Copy link
Contributor

@elimillera elimillera left a comment

Choose a reason for hiding this comment

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

Great work! Some small comments/linting

jwildfire and others added 3 commits March 19, 2022 06:04
Co-authored-by: Eli Miller <elimillera@gmail.com>
Co-authored-by: Eli Miller <elimillera@gmail.com>
Co-authored-by: Eli Miller <elimillera@gmail.com>
@jwildfire
Copy link
Contributor Author

jwildfire commented Mar 19, 2022

Thanks for the reviews @xni7 and @elimillera. I'll get chart vignette updated and then merge this once safetyCharts v0.3 is up on CRAN - going to try to clean it up and submit this week. Friendly reminder to take a look at SafetyGraphics/safetyCharts#103 if you have a chance - should be a relatively quick review!

@jwildfire
Copy link
Contributor Author

jwildfire commented Mar 19, 2022

Updated example 5 and the appendices in the chart vignette.

@xni7
Copy link
Contributor

xni7 commented Mar 19, 2022

Thanks for the reviews @xni7 and @elimillera. I'll get chart vignette updated and then merge this once safetyCharts v0.3 is up on CRAN - going to try to clean it up and submit this week. Friendly reminder to take a look at SafetyGraphics/safetyCharts#103 if you have a chance - should be a relatively quick review!

@jwildfire Just reviewed. Looks great! Just had a couple of minor comments. Btw, looks like you are an early 🐦!👍 😄

@jwildfire
Copy link
Contributor Author

All checks passing now that safetyCharts v0.3 is on CRAN. Merging to dev. Will work on cleaning up vignettes and pkgdown, and aim for v2.1 release in the next week or so.

@jwildfire jwildfire merged commit 743aa5f into dev Mar 23, 2022
@jwildfire jwildfire deleted the fix-637 branch March 23, 2022 13:34
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 this pull request may close these issues.

Migrate metadata storage to safetyCharts

3 participants