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

test: use testthat + codecov #11

Merged
merged 6 commits into from
Jan 6, 2023
Merged

Conversation

elipousson
Copy link
Collaborator

These are all fairly basic changes to start addressing #5 and #6:

  • test: use testthat + codecov
  • test: add test for amend_timestamp
  • docs: switch to Rmd for README + revise README + add package docs
  • docs: add Eli Pousson to contributors
  • refactor: add testthat, withr, and covr packages to Suggests
  • style: style timestamp.R

I can set up a GitHub action with use_github_action("test-coverage") on main once this is merged.

- test: use testthat + codecov
- test: add test for amend_timestamp
- docs: switch to Rmd for README + revise README + add package docs
- docs: add Eli Pousson to contributors
- refactor: add testthat, withr, and covr packages to Suggests
- style: style timestamp.R
@florisvdh
Copy link
Member

Hi @elipousson, just to make sure, are you waiting for me to review, or am I right that I have nothing to do yet? No hurry as far as I'm concerned.

@elipousson
Copy link
Collaborator Author

Hello @florisvdh! Apologies for the delayed reply and for not responding to your email. I was thinking it may be beneficial to merge this pull request so you could take advantage of the automated checks from code codecov.io when reviewing a second pull requests with more of the additional functionality later on. If you'd rather wait to do it all at once, that is fine as well – I could just add additional features to this pull request and let you know when it is ready to merge.

I would like to tackle the updates in at least two PRs though – one to handle the new helper functions built with RSQLite (which could be this one if we don't merge it first) and a second to incorporate the extension access functions that rely on some structured data about the GeoPackage standard. Let me know what you think!

@florisvdh
Copy link
Member

I was thinking it may be beneficial to merge this pull request so you could take advantage of the automated checks from code codecov.io when reviewing a second pull requests with more of the additional functionality later on.

OK, then I'll review this PR in the course of coming days. Thanks! I was confused by it being still a draft (= unfinished) PR, that's all.

It's perfect to split up separate topics as several successive PRs.

@elipousson elipousson marked this pull request as ready for review December 17, 2022 19:22
@elipousson
Copy link
Collaborator Author

I was confused by it being still a draft (= unfinished) PR, that's all.

Ah! That makes sense. Sorry for the mix-up.

Copy link
Member

@florisvdh florisvdh left a comment

Choose a reason for hiding this comment

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

Hi @elipousson, thanks so much for your effort and apologies for the delay. I intend to speed up my response in future PRs.

All seems fine, just some minor tweaks suggested in README.Rmd. Feel free to reword better where needed. Please re-knit since I didn't update the md file.

I generally advise to use a new line for each new sentence in (R)md files, since this makes the diffs smaller (pandoc rewraps in the output). But as I'm happy to pass the lead on this package, I'll just respect your personal preference 😉.

Please mark PRs as ready for review from the moment a PR has everything you deem needed. If still a draft (unfinished), you can ask to review specifics of course, in between.

You're welcome to merge after applying suggestions you consider useful. I look forward to the GHA setup on main!

README.Rmd Outdated Show resolved Hide resolved
README.Rmd Outdated Show resolved Hide resolved
README.Rmd Outdated Show resolved Hide resolved
README.Rmd Outdated Show resolved Hide resolved
README.Rmd Outdated Show resolved Hide resolved
@elipousson
Copy link
Collaborator Author

Thanks, @florisvdh – your suggested changes all look reasonable to me. I'll go ahead and merge it, set up a GitHub action for testing, and work to get a more fully featured pull request ready over the next month or so.

elipousson and others added 5 commits January 6, 2023 09:53
Co-authored-by: Floris Vanderhaeghe <[email protected]>
Co-authored-by: Floris Vanderhaeghe <[email protected]>
Co-authored-by: Floris Vanderhaeghe <[email protected]>
Co-authored-by: Floris Vanderhaeghe <[email protected]>
@elipousson elipousson merged commit a0c39ef into main Jan 6, 2023
@elipousson elipousson deleted the 2022-11-16_use-testthat-codecov branch January 6, 2023 14:55
@florisvdh
Copy link
Member

Thanks Eli! Note, you still have to re-knit README.Rmd in order to update README.md.

@elipousson
Copy link
Collaborator Author

Thanks for the reminder, @florisvdh. Done!

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.

2 participants