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.

Sign up for GitHub

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

Jump to bottom

Add improvments #17

Merged
merged 28 commits into from
Mar 30, 2022
Merged

Add improvments #17

merged 28 commits into from
Mar 30, 2022

Conversation

Aeilert
Copy link
Collaborator

@Aeilert Aeilert commented Mar 25, 2022

  • Add get_wb() as a short hand for getting global and regional WB stats
  • Change exposed "group" option from group_by to subgroup
  • Add temp fix for renaming of columns
  • Add a user agent to all API requests (following advice here)
  • Decide on using .onLoad (not onAttach) in zzz.R
  • Some updates to GH workflows
  • Use rds as default query format
  • Add prod url as internal package variable

Closes #11
Closes #12
Closes #6

tonyfujs and others added 19 commits March 23, 2022 12:27
@tonyfujs
move to .onAttach following message from devtools::check()
4cf4290
@tonyfujs
change default format to .rds
4cb0ac8
@tonyfujs
extract select_base_url as its own function for easier testing
31bd0d7
@tonyfujs
make prod_url an internal package variable
e6fc20b
@tonyfujs
Fix attempt::stop_if check. It was never failing due to the existence…
77fb6b4 
… of base_url
Don't export select_base_url
7df3abb
Add match.arg check for server options
705cc27
Merge pull request #13 from PIP-Technical-Team/code_review
ac1065e 
Code review
Change group_by to subgroup
3183ee3 
Closes #12
Merge pull request #15 from PIP-Technical-Team/change_group_by
9e0fed2 
Change group_by to subgroup
Use .onAttach instead of .onLoad
d99f474
Merge branch 'dev' of https://github.com/PIP-Technical-Team/pipr into…
722a800 
… dev
Add get_wb()
db7fa82
Merge pull request #16 from PIP-Technical-Team/add_get_wb
a31ee2d 
Add get_wb()
Add user agent
5f1f057
Update GH workflows
e3bcd29 
* Run R CMD checks on dev branch as well
* Only render Readme.Rmd, not other Rmd files
Revert to .onLoad
9d1d6a1
Add temp fix for renaming of response cols
1227974
Add data.table to Imports
c60332d
@Aeilert Aeilert added the enhancement New feature or request label Mar 25, 2022
@Aeilert Aeilert requested a review from tonyfujs March 25, 2022 09:16
@Aeilert Aeilert self-assigned this Mar 25, 2022
Aleksander Eilertsen added 6 commits March 25, 2022 06:22
Add get_wb() to on load caching
d849c7a
Add unit tests for caching
46e200b
Add unit tests for user_agent
4142c67
Add callr to Suggests
065ac4a
Align with pipapi updates
b52d22c 
* /versions returns a data frame by default
* /aux returns a data frame by default
* Invalid queries now correctly trigger a 404 response
Minor documentation update
141fb8e
tonyfujs
tonyfujs reviewed Mar 28, 2022
View reviewed changes
Copy link
Member

@tonyfujs tonyfujs left a comment

Choose a reason for hiding this comment

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

Hey @Aeilert
The unit tests for "Caching is enabled by default" fail for me, and it does not seem to be related to callr...

Also, what the rational for moving the messages back to .onLoad? devtools::check() still throwing a warning...

Otherwise everything looks good!

@Aeilert
Copy link
Collaborator Author

Aeilert commented Mar 28, 2022

thanks @tonyfujs

I'll check on the cache unit tests. Looks like they passed on GH, so not sure what could be wrong. But I will take a closer look to confirm.

Regarding .onLoad vs .onAttach: The startup-message should be linked to the actually triggering of the caching (based on the value of PIPR_DISABLE_CACHING), so the message needs to be wrapped inside the if-statement. In that sense I think we should either use onLoad or onAttach. But onAttach apparently does not work well with overwriting existing functionsin the namespace (which is what we do with get_stats <- memoise(get_stats) . I thus landed on .onLoad . The implication here is the caching (by default) will be enabled both when users call libary(pipr) and pipr::get_stats() (wo/ loading the library). I think that is acceptable (maybe even preferred?) .

@Aeilert
Copy link
Collaborator Author

Aeilert commented Mar 29, 2022

Hey @tonyfujs

I checked the caching unit tests and the all pass for me locally.

Aleksander Eilertsen added 2 commits March 30, 2022 02:54
Fix note in check
a618b08 
Resolve note in R CMD check. Only give start up message when package is
loaded using library().
Change default value for povline to NULL
e002c91
@Aeilert Aeilert merged commit 096f958 into main Mar 30, 2022
@Aeilert Aeilert deleted the dev branch March 30, 2022 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tonyfujs tonyfujs tonyfujs left review comments

Assignees

@Aeilert Aeilert

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
2 participants
@Aeilert @tonyfujs