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

Initial formulation of as_tibble #8

Closed
wants to merge 28 commits into
base: feature/tibble
from

Conversation

Projects
None yet
2 participants
@aedobbyn
Copy link

aedobbyn commented Sep 2, 2018

No description provided.

@aedobbyn aedobbyn referenced this pull request Sep 2, 2018

Closed

Few changes #5

@crazycapivara

This comment has been minimized.

Copy link
Owner

crazycapivara commented Oct 28, 2018

Finally I found time to check the PR and to add functions to parse all kind of responses to tibbles. I used your your first approach adding classes to the response objects, so that I could make use of the S3 system and its generic functions. Moreover, to keep all columns of the different responses I added some sort of response structure. This way all tibbles start with the same columns and additional ones are just appended. So you still have similar-ish data frames, which are easy to compare. I already merged the changes to develop. Main changes are here. If this is fine for you, I would like to add you as contributor to the DESCRIPTION file.

@crazycapivara

This comment has been minimized.

Copy link
Owner

crazycapivara commented Oct 28, 2018

Ah, and I named the function owmr_as_tibble because of the already existing as_tibble in the tibble package itself.

@aedobbyn

This comment has been minimized.

Copy link
Author

aedobbyn commented Oct 29, 2018

I used your your first approach adding classes to the response objects, so that I could make use of the S3 system and its generic functions

Nice, I like this approach!

Ah, and I named the function owmr_as_tibble because of the already existing as_tibble in the tibble package itself.

Makes sense, that function seems like something we wouldn't want to mask.

My only suggestion at the first pass would be to use library instead of require here, but of course this is very minor.

If this is fine for you, I would like to add you as contributor to the DESCRIPTION file.

Fine by me -- thanks for asking!

Let me know if you'd like me to look through anything else like the tests.

@crazycapivara

This comment has been minimized.

Copy link
Owner

crazycapivara commented Nov 1, 2018

I already added tests and updated the README file in develop. For the docs I want to switch to pkgdown. In this step I will replace require with library.

@aedobbyn

This comment has been minimized.

Copy link
Author

aedobbyn commented Nov 1, 2018

I already added tests

Yeah sorry what I linked to up there in was the test errors on Travis. I can look into them if you like.

For the docs I want to switch to pkgdown. In this step I will replace require with library.

Sounds great!

@crazycapivara

This comment has been minimized.

Copy link
Owner

crazycapivara commented Nov 2, 2018

I managed to merge the new features to master and to switch to pkgdown-docs.

@aedobbyn

This comment has been minimized.

Copy link
Author

aedobbyn commented Nov 2, 2018

Nice! Let me know if you want me to do any work on the docs.

@crazycapivara

This comment has been minimized.

Copy link
Owner

crazycapivara commented Nov 21, 2018

Hi, sorry for the delay in my reply. Thanx a lot for your great input. If you have some suggestions on the docs it is always welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment