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 get sql function #68

Merged
merged 23 commits into from
Mar 27, 2024
Merged

Add get sql function #68

merged 23 commits into from
Mar 27, 2024

Conversation

cjrace
Copy link
Contributor

@cjrace cjrace commented Mar 9, 2024

Brief overview of changes

Add a get_clean_sql function to resolve #55, and details on code coverage setup to resolve #53.

This increases the package version to 0.2.0.

Why are these changes being made?

This function is used across lots of DfE projects already. Other bits of tidy up to resolve known issues with documentation.

Detailed description of changes

Updates to contributing page:

  • Typo fix on usethis::use_test()
  • Extra information on TDD approach and roxygen2 documentation
  • Typo fix on lintr::lint_package()
  • Reformat keyboard shortcuts section so it's easier to see
  • Add details on test coverage checks
  • Add instructions for adding a new vignette

Updates to package documentation:

  • Added titles into _pkgdown.yml to group functions in reference list

Added get_clean_sql() function, documentation and tests. Includes a sample SQL script to test with.

Added vignette for connecting to SQL to give more user friendly context for get_clean_sql().

Bumping package version to 0.2.0 for this and #59.

Added note to README pointing to the reference documentation for the package.

Additional information for reviewers

  1. Use the following line to preview package documentation updates locally:
devtools::build_site()
  1. I will squash the commits when merging as I got into a slight mess with git rebase conflicts that seems to have duplicated commits in the commit history.

  2. I'll reflect any changes here in Fix typo and update guidance to point to dfeR package analysts-guide#43 to update the relevant section of the Analysts' Guide.

Issue ticket number/s and link

#55, #53.

@cjrace cjrace linked an issue Mar 9, 2024 that may be closed by this pull request
@cjrace cjrace marked this pull request as ready for review March 11, 2024 09:59
@cjrace
Copy link
Contributor Author

cjrace commented Mar 11, 2024

Copy link
Contributor

@rmbielby rmbielby left a comment

Choose a reason for hiding this comment

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

Mainly just a few minor tweaks to the text.

vignettes/connecting_to_sql.Rmd Show resolved Hide resolved
DESCRIPTION Show resolved Hide resolved
vignettes/connecting_to_sql.Rmd Outdated Show resolved Hide resolved
vignettes/connecting_to_sql.Rmd Outdated Show resolved Hide resolved
vignettes/connecting_to_sql.Rmd Outdated Show resolved Hide resolved
vignettes/connecting_to_sql.Rmd Outdated Show resolved Hide resolved
@cjrace cjrace requested a review from rmbielby March 23, 2024 15:58
@cjrace cjrace merged commit abd6e19 into main Mar 27, 2024
10 checks passed
@cjrace cjrace deleted the add-get_sql branch March 27, 2024 10:38
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.

Add in read_sql() type function Add contributing guidelines
2 participants