-
Notifications
You must be signed in to change notification settings - Fork 27
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
draft of use_crate() #361
draft of use_crate() #361
Conversation
Co-authored-by: Josiah Parry <[email protected]>
Co-authored-by: Josiah Parry <[email protected]>
Do we need all two files, 400 lines each? Can we trim it only to used functions? |
Possibly, but if we intend to incrementally update other parts of rextendr to use these standalone checks, maybe we should think about which ones we are likely to use elsewhere (not just for this function) and then trim everything else. What do you think, @JosiahParry? |
I think including the files is helpful for the future development of the package. I would suggest to keep them as is. This is because they add no other dependencies hence "standalone." Together they are 20kb which is very small. If size is a concern we can remove comments and whitespace. But again, this is a utility package, not one that is included in the final binary of another. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀 Thank you for making this change. This alone will simplify the process of adding crates to one's Cargo.toml
lowering the bar to entry for writing Rust powered R packages.
If the standalone checks become an issue later while running checks prior to new version release we can revisit.
However, I think that they provide a significant QOL improvement that should not be overlooked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is fine as is right now.
I don't see a need to hand modify these functions and helpers to fit neatly in with this PR solely. It is a tool discovered by this PR, and should just be added if we like it.
Awesome! Thanks! If you all want someone to work on integrating/modifying the standalone checks more, I can probably devote some time to it next month. |
@kbvernon , could you please also update |
can do! thanks, @Ilia-Kosenkov! |
) | ||
|
||
# clear empty options | ||
cargo_add_opts <- purrr::discard(cargo_add_opts, rlang::is_empty) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change broke the function. Looks like we might be missing a test for this. I installed the new version
rextendr::use_crate("lazy-static")
Running cargo add lazy-static '--features ' '--optional false'
Error:
! ! System command 'cargo' failed
This is because "--features "
is included in the vector that is passed to processx. --features
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, i see this:
purrr::discard(list(a = "", b = 1, c = NULL), rlang::is_empty)
# $a
# [1] ""
#
# $b
# [1] 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and
paste(NULL)
# character(0)
paste(NULL, collapse = " ")
# ""
For #360