-
Notifications
You must be signed in to change notification settings - Fork 1
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
Package Review and CMD Check #26
Comments
This is + traceback: Error in fromJSON(file = json_file) :
either json_str or file must be supplied to fromJSON
3. stop("either json_str or file must be supplied to fromJSON")
2. fromJSON(file = json_file) at domain_mapping.R#38
1. domain_mapping() |
@RayStick Very stupid question but am I supposed to provide the json_file inside the function when I run the function. I think I have been running it without json_file! |
Let me check :D |
This is what I have been running: library(devtools)
devtools::load_all()
?domain_mapping
domain_mapping() but now that I run: domain_mapping(,, TRUE) it's working, yah!!! Another question, is there a reason why the 3rd argument isn't by default TRUE, is this to avoid another issue? |
Great =D What this TRUE means is Does that make sense? Do you think there is a better way to approach it? |
Maybe I can code something that says if no input arguments are given, it triggers demo mode. And remove this argument altogether? |
It makes total sense - thank you for the clarification! I am trying to recall examples where R packages have similar approach, it's usually the 1st argument that is used to trigger example. Let me sleep on this and see what is best practices! I am very happy that it's working now! |
Yes, we can return to this, no rush. Would be good to do what is typical for an R package, so let me know! |
I am looking into the 2nd point with CMD check - no need to reply today - I am just adding my points and questions to come back to it. I don't see that any of the dependencies mentioned in DESCRIPTION file. Also, I would also use |
To summarize a few points from our call. Before public release we want:
Perhaps a PR for each? |
Thank you, yes, it makes sense to have 2 PRs. |
As mentioned in #22, we need to address two issues to make sure all function work before creating any docs:
domain_mapping()
function is workingCurrent error:
Current errors:
The text was updated successfully, but these errors were encountered: