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

Issue 9: Getting started vignette #32

Merged
merged 19 commits into from
May 23, 2024
Merged

Issue 9: Getting started vignette #32

merged 19 commits into from
May 23, 2024

Conversation

athowes
Copy link
Collaborator

@athowes athowes commented May 14, 2024

Description

This PR will close #9.
It is a draft of a rewritten version of the first "Get started" vignette.

Checklist

  • My PR is based on a package issue and I have explicitly linked it.
  • I have included the target issue or issues in the PR title in the for Issue(s) issue-numbers: PR title
  • I have read the contribution guidelines.
  • I have tested my changes locally.
  • I have added or updated unit tests where necessary.
  • I have updated the documentation if required.
  • My code follows the established coding standards.
  • I have added a news item linked to this PR.
  • I have reviewed CI checks for this PR and addressed them as far as I am able.

@seabbs
Copy link
Contributor

seabbs commented May 14, 2024

This looks good so far. Give me a ping when you want a proper review

@athowes
Copy link
Collaborator Author

athowes commented May 17, 2024

@seabbs @kgostic -- I'm not fully done with this but would be interested in some review / feedback to make sure that I'm staying on the right path.

If you'd prefer to wait until the vignette is more fully complete, then let me know.

@athowes athowes mentioned this pull request May 20, 2024
@athowes
Copy link
Collaborator Author

athowes commented May 20, 2024

I will now try fixing this pkgdown error so that you should hopefully be able to extract out the .html version of the vignette.

@athowes athowes force-pushed the first-vignette branch 2 times, most recently from cf51125 to d3cc70a Compare May 20, 2024 15:57
@athowes
Copy link
Collaborator Author

athowes commented May 20, 2024

I don't know what is causing the issue with the pkgdown website on check. I can render it OK locally. Let me know if you have any idea. It is also there on the other PR so it must be something already in main.

@kgostic
Copy link
Collaborator

kgostic commented May 20, 2024

@seabbs -- can you help with this tomorrow?

kgostic
kgostic previously approved these changes May 20, 2024
Copy link
Collaborator

@kgostic kgostic left a comment

Choose a reason for hiding this comment

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

@athowes this looks great overall. Your writing is really clear and concise.

I do think there's a bit at the end that still needs to be fleshed out, but if you can finish off the end, I think this is a reasonable outline for a getting started vignette.

@seabbs may want to add a bit more about installation and contributing issues at the top.

I'm approving so as not to hold things up but it would be really nice to see a rendered version of the makrdown as I haven't yet seen the figures in situ.

vignettes/epidist.Rmd Show resolved Hide resolved
vignettes/epidist.Rmd Outdated Show resolved Hide resolved
vignettes/epidist.Rmd Outdated Show resolved Hide resolved
vignettes/epidist.Rmd Outdated Show resolved Hide resolved
vignettes/epidist.Rmd Show resolved Hide resolved
vignettes/epidist.Rmd Outdated Show resolved Hide resolved
@seabbs
Copy link
Contributor

seabbs commented May 21, 2024

Can you rebase this so the pkgdown builds and then I will review (I thought about rebasing myself but it seemed a bit cheeky)

@athowes
Copy link
Collaborator Author

athowes commented May 21, 2024

@seabbs may want to add a bit more about installation and contributing issues at the top.

Agree on making sure people have the package installed, but would be tempted to have that displayed very prominently in the README and not in the "getting started" vignette. As well, FWIW and could be convinced, but I don't guess that the top of this vignette is the right place for anything long about contributing issues. Could be open to a short one sentence about it.

@seabbs
Copy link
Contributor

seabbs commented May 21, 2024

Agree on making sure people have the package installed, but would be tempted to have that displayed very prominently in the README and not in the "getting started" vignette.

Agree

@athowes athowes changed the title Issue 9: Rewrite first vignette Issue 9: Getting started vignette May 21, 2024
vignettes/epidist.Rmd Outdated Show resolved Hide resolved
vignettes/epidist.Rmd Outdated Show resolved Hide resolved
vignettes/epidist.Rmd Outdated Show resolved Hide resolved
vignettes/epidist.Rmd Outdated Show resolved Hide resolved
vignettes/epidist.Rmd Outdated Show resolved Hide resolved
vignettes/epidist.Rmd Outdated Show resolved Hide resolved
vignettes/epidist.Rmd Outdated Show resolved Hide resolved
seabbs
seabbs previously approved these changes May 21, 2024
Copy link
Contributor

@seabbs seabbs left a comment

Choose a reason for hiding this comment

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

I really like this @athowes it is really clearly written and communicates the points very well.

I put a few minor comments. My only other concern here is maybe length but uncertain.

@seabbs
Copy link
Contributor

seabbs commented May 23, 2024

When ready for another review cna you reping using the interface at top right (as this gives a different form of notification that I filter for vs saying something like ready for review)

@athowes
Copy link
Collaborator Author

athowes commented May 23, 2024

Great, thanks all for the comments both.

I'm going to give this another hour or so try, and then I think we should clear it up and merge it in to get things moving. Happy to come back and iterate it again in future.

Have enjoyed writing this and the comments / feedback process by the way! Very smooth / logistically easy to handle.

@seabbs
Copy link
Contributor

seabbs commented May 23, 2024

Actions points I see are:

  • Decide on if telling people about packages: did tell people
  • Add gt to the suggests: done
  • Decide on use of posterior vs from scratch output extraction: I don't know how to do it in posterior so doing from scratch
  • New issue for looking again at plotting to either use package plots, remove package plots or standardise themes: created issue Update figures in vignettes #55
  • Move @athowes from a contributor to an author in the DESCRIPTION: athowes is already "aut" (?)
  • Close all resolved conversations with clarification if useful: done, including creating issue Add right truncation figure to getting started vignette #56
  • Fix linting issues
  • Make ready for review and ping

@seabbs seabbs marked this pull request as ready for review May 23, 2024 19:52
Copy link
Contributor

@seabbs seabbs left a comment

Choose a reason for hiding this comment

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

LGTM. This looks to have covered all review points from myself and @kgostic.

@seabbs seabbs added this pull request to the merge queue May 23, 2024
Merged via the queue into main with commit f59b96e May 23, 2024
3 of 4 checks passed
@seabbs seabbs deleted the first-vignette branch May 23, 2024 19:56
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.

Expand getting started vignette
3 participants