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.

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

Tibble output via tibblify #5

Draft
wants to merge 137 commits into
base: master
Choose a base branch
from
Draft

Conversation

krlmlr
Copy link
Contributor

@krlmlr krlmlr commented Aug 2, 2021

The tibblify package allows for a much more robust approach than jsonlite::fromJSON(simplifyVector = TRUE) . We can infer a schema from the data (as R code) and then use that schema when coercing the data to a tibble. This gives type stability at the highest level possible, while automating the generation of boilerplate code.

I used the following workflow:

  • Query objects from the demo API (currently only teams and spaces)
  • Use tibblify::tibblify() to automatically convert to a tibble, double-check
  • Use tibblify::get_spec() to get the specification (=schema)
  • Implement a cuf_() function
  • Add a test for that function
  • Rinse and repeat

This PR is not how PRs should be done. Unfortunately, the commits build one upon another. Can you please review the commits individually and let me know which you don't agree with?

How do we proceed? Would you like to pick up from here, do you need more help?

@mgirlich: I'm not sure why I had to do 7a93fc7 manually, why this wasn't inferred from the data.

@krlmlr krlmlr mentioned this pull request Aug 2, 2021
@psolymos
Copy link
Owner

psolymos commented Aug 2, 2021

Thank you @krlmlr this looks like a nice way of tibblifying the responses. Let me check your additions and try to do the same for a few more endpoints to see if anything unexpected comes up with the approach.

I also like the testing to be based on the mock Apiary server. This can be added to the cu_ functions too.

I am also wondering if adding the cuf_ functions can be automated using your workflow to add new functions and tests. I would like to either (1) keep related functions in the same file, or (2) be able to sort them to see those next to each other (cu-xxx.R and cu-xxx-tb.R for alphabetical sorting).

@krlmlr
Copy link
Contributor Author

krlmlr commented Aug 2, 2021

Thanks.

I added a few tests for cu_*() .

I only have a semi-automatic way of adding the cuf_*() wrappers -- see script/tibblify.R. Most likely we need to refine what tibblify::get_spec() returns; we also want to e.g. convert ClickUp times to proper times. I kept related functions in the same file.

@psolymos
Copy link
Owner

psolymos commented Aug 2, 2021

The date handling can be done as lcol_dat("dob", .parser = ~ cu_date_from(.x)) [not tested, just inferred from docs].

Do you think the inverse-tibblify could work for POST requests? Or should we restrict the scope here to GET requests?

@krlmlr
Copy link
Contributor Author

krlmlr commented Aug 2, 2021

Let's focus on GET first, need to understand the best structure for POST and friends.

@krlmlr
Copy link
Contributor Author

krlmlr commented Aug 2, 2021

One more thing: It might be useful to export the step that converts a cu_*() list to a tibble in a separate function, to aid debugging and reprexing.

@mgirlich
Copy link

mgirlich commented Aug 3, 2021

@krlmlr Probably it was actually inferred from the data but just not printed as there was a bug in the printing :-/
Can you try out with the dev version?

@krlmlr
Copy link
Contributor Author

krlmlr commented Sep 22, 2021

I think the following getters would be most important in the beginning:

  • cu_get_folders()
  • cu_get_lists()
  • cu_get_tasks()
  • cu_get_time_entries_within_date_range()
  • cu_get_singular_time_entry()
  • cu_get_list_members()

@krlmlr
Copy link
Contributor Author

krlmlr commented Oct 2, 2021

I'll take another stab here.

@krlmlr krlmlr marked this pull request as draft October 2, 2021 07:12
@krlmlr
Copy link
Contributor Author

krlmlr commented Oct 3, 2021

I added https://github.com/krlmlr/clickrup/blob/f-data-frame/script/README.md that describes the process and the next steps.

@krlmlr
Copy link
Contributor Author

krlmlr commented Oct 3, 2021

It looks like a few more days of work to complete this, including a test workspace and automated tests.

@krlmlr
Copy link
Contributor Author

krlmlr commented Oct 3, 2021

It looks like the mock API is still available, via https://private-anon-bcbe6af027-clickup20.apiary-mock.com/api . Need to double-check the access token.

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.

3 participants