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

guess_fileSize should not cause create_codemeta() to fail just bc a dependency isn't installed. #196

Closed
maelle opened this issue Nov 22, 2018 · 11 comments

Comments

@maelle
Copy link
Member

maelle commented Nov 22, 2018

but on a Linux server, for https://github.com/ropensci/infx

codemetar::create_codemeta("repos/ropensci/infx")
Error in run(bin, args = real_cmdargs, stdout_line_callback = real_callback(stdout),  :
  System command error

Any idea what could be the reason for this @cboettig?

@maelle maelle added the bug label Nov 22, 2018
@cboettig
Copy link
Member

cboettig commented Dec 3, 2018

Looks like this error occurs when devtools::build fails due to a missing dependency. (you'd think it would be possible to build a package without dependencies installed, but apparently not, even with vignettes=FALSE. (Also we should probably replace devtools::build with pkgbuild::build eventually).

I suppose we could call devtools::install() ahead of building, but I think the better behavior would by to wrap the guess_fileSize function in a tryCatch so it just fails gracefully if the package cannot be built.

Really, the whole idea of building a package to guess fileSize is a nice statistic to have, but also a potentially fragile and computationally expensive operation and so should both fail gracefully and have a way to toggle off. I'll adjust this issue title to reflect these outstanding issues.

https://github.com/ropensci/codemetar/blob/master/R/guess_metadata.R#L225

  • guess_fileSize should fail gracefully if package cannot be built.
  • User should be able to easily opt of out of guess_fileSize behavior (perhaps with a default behavior set as an option so it's easy to toggle globally too).

Thoughts @maelle ?

@cboettig cboettig changed the title Error I don't get locally guess_fileSize should not cause create_codemeta() to fail just bc a dependency isn't installed. Dec 3, 2018
@maelle
Copy link
Member Author

maelle commented Dec 3, 2018

Do you use the dev branch? Devtools should have been replaced in it

@maelle
Copy link
Member Author

maelle commented Dec 3, 2018

Agree with the changes proposed

@cboettig
Copy link
Member

cboettig commented Dec 3, 2018

👍 no, sorry, I ran the above checks with master. Are you seeing this error with the dev version? Are we due for a PR and new release soon?

@maelle
Copy link
Member Author

maelle commented Dec 4, 2018

Yes there is a PR from dev to master, not sure yet what should be the criteria for the release i.e what to fix/improve before merging?

@maelle
Copy link
Member Author

maelle commented Dec 4, 2018

And i only use the dev version nowadays

@cboettig
Copy link
Member

cboettig commented Dec 4, 2018

Sounds like we should merge dev to master if that's what you're using anyway! And plan a release soon?

I have simple criteria for a release workflow:

  • I like to have any dev merged into master once they are passing tests and implemented a feature (i.e. to keep PRs small; basically the same way you do for branch->dev without the extra abstraction).
  • I like to release to CRAN whenever (a) master is better than CRAN's version, (b) we've merged most open PRs / are not planning any writing/merging new PRs soon, and (c) we're release ready (updated NEWS and passing tests, haven't just had a CRAN release).

So I think :shipit: ?

@maelle
Copy link
Member Author

maelle commented Dec 5, 2018

What I implemented is not a global option to toggle though.

@maelle maelle removed the bug label Dec 18, 2018
@maelle
Copy link
Member Author

maelle commented Dec 18, 2018

@cboettig do you have a favorite example of a package with such a toggle option?

@cboettig
Copy link
Member

not off hand, but I was thinking just setting something in options. e.g. options(codemetar_build_package = FALSE). Then the function can do something like build <- getOptions(codemetar_build_package, TRUE) to default to building.

@maelle
Copy link
Member Author

maelle commented Nov 13, 2019

I think we can close thanks to the default behavior introduced in #239

@maelle maelle closed this as completed Nov 13, 2019
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

No branches or pull requests

2 participants