search

r-spatial / rgeopackage

R package with helper tools in creating or handling GeoPackage files
GNU General Public License v3.0
10 stars 1 forks source link

test: use testthat + codecov #11

Closed elipousson closed 1 year ago

elipousson commented 2 years ago

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

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

florisvdh commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

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

elipousson commented 1 year ago

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.

florisvdh commented 1 year ago

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

elipousson commented 1 year ago

Thanks for the reminder, @florisvdh. Done!