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

Remove dependencies? #1

Closed
etiennebacher opened this issue Jan 16, 2024 · 3 comments · Fixed by #2
Closed

Remove dependencies? #1

etiennebacher opened this issue Jan 16, 2024 · 3 comments · Fixed by #2

Comments

@etiennebacher
Copy link
Contributor

etiennebacher commented Jan 16, 2024

Hi, that looks like a great example for making an extendr-powered package, thank you!

I don't have a direct usecase for this but I suppose several other packages could use it under the hood. base64enc doesn't have any deps so I think this package could be more attractive by also having 0 dependencies, is this something you would consider?

From what I can see rlang and cli should be easy to remove, but blob brings several deps with it. Is blob really necessary?

@JosiahParry
Copy link
Collaborator

0 dependencies is really easy to accomplish. The current deps are fairly small hence why I felt comfortable with them. Otherwise, yah, it is really easy to make this a 0 dep package and we can do so no problem.

howeverrrr.... blob should definitely be a suggested package b/c blob is just structure(list(), class = c("blob", "vctrs_list_of", "vctrs_vctr", "list" )). b64 doesn't need to have blob imported to assign the classes to a list object and the blob class makes for compatibility with data.frames otherwise it is just a list of raw vectors.

Do you want to try adjusting it to be 0 dep for a 0.1.1 release?

@etiennebacher
Copy link
Contributor Author

Do you want to try adjusting it to be 0 dep for a 0.1.1 release?

Sure I can give it a try later today (might need help for the blob stuff though)

@JosiahParry
Copy link
Collaborator

Ya, no problem. One of the other reasons for choosing a blob was for type consistency. These functions should always return the same type. Since its not possible to return multiple raw vectors outside of a list, the functions should always return a list of raws (and in this case with the blob classes)

One of the important places to change is here:

if !r.inherits("blob") {

right now it requires that blob type be passed in. Instead of checking the class of the list, you can instead check the class of each element. If it doesn't inherit raw an NA can be returned.

Setting the blob class is always done before the list is returned. That can probably remain since class is just an attribute and having it does not change the behavior of the list. It just gets a fancier print method when blob is imported.

.set_class(&["blob", "vctrs_list_of", "vctrs_vctr", "list"])

Here is where blob is forcibly added to the NAMESPACE

#' @importFrom blob blob

I think that's about it

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 a pull request may close this issue.

2 participants