Skip to content

#455 database refperiod (#3)#456

Draft
dsvedberg wants to merge 1 commit intoStatisticsGreenland:mainfrom
dsvedberg:main
Draft

#455 database refperiod (#3)#456
dsvedberg wants to merge 1 commit intoStatisticsGreenland:mainfrom
dsvedberg:main

Conversation

@dsvedberg
Copy link

Changes both REFPERIOD and DATABASE into metadata for the entire table. Since they are language dependent, they are placed in table2.

  • Updates NEWS with new keywords.

  • Updates _pkgdown.yml with new functions

  • Updates URL to PxJob download/documentation

Updates redirected URL in README.Rmd

To-do

  • Update version number
  • Update NEWS.md

* StatisticsGreenland#455 Implements px_refperiod and px_database

Changes both REFPERIOD and DATABASE into metadata for the entire table. Since they are language dependent, they are placed in table2.

* Updates NEWS with new keywords.

* Updates package docs with new functions, _pkgdown.yml

* Updates URL to PxJob download

Updates redirected URL.
@dsvedberg
Copy link
Author

Took the liberty to change some inconsistent single/double-quotes for strings in the test file and updated a redirected URL.

I realize they should probably be separate commits, just let me know if I should redo it with only changes necessary for px_database and px_refperiod.

@johan-ejstrud
Copy link
Collaborator

@dsvedberg it looks great! You've added tests, updated NEWS, updated README and everything. All tests passes. Great stuff. 🥳

I only had one review comment (to the NEWS file).

Also, as you mention, it get's a bit messy when changing formatting at the same time as code changes - it becomes hard to see what the functional code changes are. For another time, I would split it up yes, so I would have one commit "Align quotation marks to use double quotes", and "Change indentation", but this is nitpicking. Your choice.

As soon as you have moved the comment in NEWS, you can run usethis::use_version("dev') which will increment the dev version number. I then think we are ready to merge. 🤩

@johan-ejstrud
Copy link
Collaborator

Oh, also, you probably discovered some undocumented things about how this repo works.

As you see, the documentation isn't detailed about "how to work" with the repo, because I've been making most of the changes myself. Feel free to add documentation/comments that would be helpful for potential future contributors.

@dsvedberg
Copy link
Author

@dsvedberg it looks great! You've added tests, updated NEWS, updated README and everything. All tests passes. Great stuff. 🥳

I only had one review comment (to the NEWS file).

Also, as you mention, it get's a bit messy when changing formatting at the same time as code changes - it becomes hard to see what the functional code changes are. For another time, I would split it up yes, so I would have one commit "Align quotation marks to use double quotes", and "Change indentation", but this is nitpicking. Your choice.

As soon as you have moved the comment in NEWS, you can run usethis::use_version("dev') which will increment the dev version number. I then think we are ready to merge. 🤩

Squashed the commits in my fork... Ssince I'm doing a new PR I'll just leave them as separate commits. Will get back with a new PR next week!

@johan-ejstrud
Copy link
Collaborator

Sweet. Looking forward to it. Have a great weekend. 👋

@johan-ejstrud johan-ejstrud marked this pull request as draft March 13, 2026 10:26
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