Skip to content
This repository has been archived by the owner on Oct 10, 2022. It is now read-only.

Various improvements #8

Merged
merged 22 commits into from
Apr 5, 2018
Merged

Various improvements #8

merged 22 commits into from
Apr 5, 2018

Conversation

maelle
Copy link
Contributor

@maelle maelle commented Mar 21, 2018

Next time I should try to use gitflow. 👼

  • I am not able yet to reproduce the error from CRAN 😢 In the latest Travis build the failing job is the one on R devel on OSX... https://travis-ci.org/maelle/HIBPwned/builds/356264582

  • I have added some trying and waiting in utils.R

  • I have added grouping to the reference in the pkgdown website

  • I have taken the freedom to use styler and lintr to mostly shorten lines, add space, and rename "URLS" to "urls" (I added exceptions for non snake case function names)

@maelle
Copy link
Contributor Author

maelle commented Mar 21, 2018

I could tacke #5 (breaking change though...) and #6 if you agree with them.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 80199ee on maelle:master into 8f6e86c on stephlocke:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 80199ee on maelle:master into 8f6e86c on stephlocke:master.

@coveralls
Copy link

coveralls commented Mar 21, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling b0059ba on maelle:master into 8f6e86c on stephlocke:master.

@stephlocke
Copy link
Collaborator

I keep URLS uppercase as it avoids naming collisions with some functions that exist in different packages. I'd prefer it to stay as is if that's ok.

@maelle maelle requested a review from stephlocke March 21, 2018 15:41
@maelle
Copy link
Contributor Author

maelle commented Mar 26, 2018

This could be merged but I still get a failing build on OSX R devel (see this log for instance) because of missing dependencies, and I still wasn't able to reproduce the CRAN error.

@maelle
Copy link
Contributor Author

maelle commented Apr 4, 2018

@stephlocke

  • Can I merge this? The failure on Mac R devel seems to be a Travis problem, not a R problem.

  • Does the package need anything more before the next CRAN release? this PR closes all 3 open issues, and the CRAN note will disappear.

@stephlocke
Copy link
Collaborator

Travis fail? https://travis-ci.org/stephlocke/HIBPwned/jobs/358321151
Is it possible to write a test for the get and retry funciton you made?

@maelle
Copy link
Contributor Author

maelle commented Apr 4, 2018

the Travis fail seems to be a Travis issue (it fails to install dependencies on Mac R devel). I could investigate it or remove the Mac R devel build from the matrix?

@stephlocke
Copy link
Collaborator

Yeah, I don't really care all that much about mac ;)

Copy link
Collaborator

@stephlocke stephlocke left a comment

Choose a reason for hiding this comment

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

Couple of minor questions on this.

Imports:
httr,
urltools,
jsonlite,
ratelimitr
RoxygenNote: 6.0.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason why you've incremented for the dev version here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No I should change this

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now I know I have a reason 😿

VignetteBuilder: knitr
URL: https://github.com/stephlocke/HIBPwned
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe better to just refer to the pkgdown site

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One can do both, and as a potential contributor and stargazer I prefer seeing the repo ☺

Copy link
Collaborator

Choose a reason for hiding this comment

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

But we have the github in the bugs field, but meh, I don't have particularly strong feelings about this so sure thing, we'll go with both

@stephlocke stephlocke merged commit 8933f8e into jumpingrivers:master Apr 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants