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 database vignette #311

Merged
merged 9 commits into from
May 23, 2024
Merged

add database vignette #311

merged 9 commits into from
May 23, 2024

Conversation

sbfnk
Copy link
Contributor

@sbfnk sbfnk commented May 17, 2024

Closes #296.

  • Please check if the PR fulfills these requirements
  • I have read the CONTRIBUTING guidelines
  • A new item has been added to NEWS.md
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • Checks have been run locally and pass
  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

It adds a prototype for a database vignette

  • Other information:

Format etc. can be discussed as well as whether this is desirable to have. Creating this PR as a suggestion for what the database might look like.

Copy link

This pull request:

  • Adds 7 new dependencies (direct and indirect)
  • Adds 2 new system dependencies
  • Removes 0 existing dependencies (direct and indirect)
  • Removes 0 existing system dependencies

(Note that results may be inacurrate if you branched from an outdated version of the target branch.)

@joshwlambert
Copy link
Member

Thanks @sbfnk this looks great. I'm happy to move forward with this change and merge into main. A couple of points before merging.

  1. When I checked that the pkgdown site would render it correctly (pkgdown::build_site()), the table was not showing. I believe this is because {DT} tables are HTML widgets which requires pkgdown as_is: true to be removed from the vignette header. Are you finished pushing to this branch? If so I'll make this change before merging to make sure the vignette renders correctly on the website.

  2. In your original issue you said

so that people can check if there are useful distributions in there for them without having to install the package.

If we want this to be only on the website and not shipped with the package we could consider converting this from a vignette to an article. We can always change this down the road so happy to merge before we've decided on this point.

@sbfnk
Copy link
Contributor Author

sbfnk commented May 23, 2024

  1. Yes, please go ahead.
  2. That sounds like a good idea to me - presumably e.g. on CRAN the table wouldn't be rendered either

@jamesmbaazam
Copy link
Member

This is a nice addition and might be worth evaluating its benefit over the suggested dashboard in #263 or what gap the dashboard would fill that this database doesn't.

@joshwlambert
Copy link
Member

presumably e.g. on CRAN the table wouldn't be rendered either

I wasn't sure whether it would render on CRAN, according to this page I think it does: https://cran.r-project.org/web/packages/DT/vignettes/DT.html. I'll leave as a vignette for now and if we think it is better for this to only be on the website and not on CRAN we can convert to an article in a subsequent PR.

This is a nice addition and might be worth evaluating its benefit over the suggested dashboard in #263 or what gap the dashboard would fill that this database doesn't.

I see these two things as positive duplication. There will be some overlap between this vignette and the dashboard, but 1) it doesn't hurt to provide the ability to search through the database in multiple places, 2) the dashboard is still in development and this provides a quick solution, 3) the dashboard will provide more functionality that just searching through the database.

Copy link

This pull request:

  • Adds 7 new dependencies (direct and indirect)
  • Adds 2 new system dependencies
  • Removes 0 existing dependencies (direct and indirect)
  • Removes 0 existing system dependencies

(Note that results may be inacurrate if you branched from an outdated version of the target branch.)

1 similar comment
Copy link

This pull request:

  • Adds 7 new dependencies (direct and indirect)
  • Adds 2 new system dependencies
  • Removes 0 existing dependencies (direct and indirect)
  • Removes 0 existing system dependencies

(Note that results may be inacurrate if you branched from an outdated version of the target branch.)

@epiverse-trace epiverse-trace deleted a comment from github-actions bot May 23, 2024
@epiverse-trace epiverse-trace deleted a comment from github-actions bot May 23, 2024
Copy link

This pull request:

  • Adds 7 new dependencies (direct and indirect)
  • Adds 2 new system dependencies
  • Removes 0 existing dependencies (direct and indirect)
  • Removes 0 existing system dependencies

(Note that results may be inacurrate if you branched from an outdated version of the target branch.)

@joshwlambert
Copy link
Member

Apologies for the spam. It took a few commits to get the lintr workflow to pass.

This PR is now ready to be merged.

@joshwlambert joshwlambert merged commit 63e9890 into main May 23, 2024
10 checks passed
@joshwlambert joshwlambert deleted the database branch May 23, 2024 15:12
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.

Database vignette
3 participants