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

Export function to read from zotero database #37

Merged
merged 3 commits into from
Sep 6, 2018

Conversation

Robinlovelace
Copy link
Contributor

If you do this it will make it easy to load the database in the R session rather than in shiny which blocks your entire RStudio session - see #36. It's also a useful function so deserves to be exported IMO. Great work, cannot figure out how it all works, but it's bloody awesome IMO.

@crsh
Copy link
Owner

crsh commented Aug 16, 2018

Hi Robin,

two thoughts:

  1. I agree that the sometimes long loading is a nuisance and needs to be addressed. I feel like exporting the function falls a little short of a satisfactory fix because it requires users to know that they need to run this function by hand in advance. I don't mind exporting the function (and it's probably a decent quick fix) but I don't think it's a proper fix. I think a viable approach might be to move loading of the library outside of the shiny app, that is before this line:

ui <- miniPage(

If I find some time I'll look into it, but I'm currently swamped with other things, so it may take a while.

  1. The documentation is missing descriptions of the function arguments. That's why the Travis checks are failing. Would you be willing to add those as well?

@Robinlovelace
Copy link
Contributor Author

Hi @crsh thanks for taking a look. This PR is just to illustrate what did to use that function in my own work. Hope was it would be useful with respect to #36 but definitely not sufficient to fix it. I think your suggestion of loading it before the shiny app captures the RStudio suggestion is sensible. I agree 100%!

I'm happy to add some documentation to the newly exported function - will work on that. Just wanted to check you were happy to export it.

@Robinlovelace
Copy link
Contributor Author

Job done on the documentation of parameters I think. Passed devtools::check() on my computer. Hope it succeeds on Travis!

@crsh crsh merged commit c6143b0 into crsh:master Sep 6, 2018
@Robinlovelace
Copy link
Contributor Author

Thanks. Feel free to change what I've done, not sure how this will integrate in the wider plan to make citr not prevent use of RStudio while the (potentially large) bibliography is being loaded-in.

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