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

append() drops custom datapackage class #198

Open
2 tasks
peterdesmet opened this issue Mar 27, 2024 · 5 comments
Open
2 tasks

append() drops custom datapackage class #198

peterdesmet opened this issue Mar 27, 2024 · 5 comments
Labels
bug Something isn't working complexity:high Likely complex to implement
Milestone

Comments

@peterdesmet
Copy link
Member

peterdesmet commented Mar 27, 2024

When using append(), the datapackage lists looses its custom class:

library(frictionless)
p <- create_package()
class(p)
#> [1] "datapackage" "list"
p <- append(p, c(foo = "bar"))
class(p)
#> [1] "list"

Created on 2024-03-27 with reprex v2.1.0

This results in check_package() warnings like in #194. I assume the way to solve this is by creating a append.datapackage() function.

To update:

@peterdesmet peterdesmet added the bug Something isn't working label Mar 27, 2024
@peterdesmet peterdesmet added the complexity:low Likely not complex to implement label Jul 3, 2024
@peterdesmet peterdesmet added this to the 1.2.0 milestone Jul 8, 2024
@PietrH PietrH self-assigned this Jul 9, 2024
@PietrH PietrH added complexity:high Likely complex to implement and removed complexity:low Likely not complex to implement labels Jul 9, 2024
@PietrH
Copy link
Member

PietrH commented Jul 9, 2024

I'm struggeling because append() is not an S3 generic:

> sloop::is_s3_generic("append")
[1] FALSE

Thus creating a method for it is not as strait forward as it was for print()

@PietrH
Copy link
Member

PietrH commented Jul 9, 2024

Something something methods::setGeneric() ?

@peterdesmet
Copy link
Member Author

@khusmann would you know how to extend base R append() for a specific S3 class? Cf. print() we would like to avoid a masking warning when loading the package.

@PietrH PietrH removed their assignment Jul 10, 2024
@khusmann
Copy link
Contributor

I don't think there's any good way of doing this. The standard way of doing this, from what I've seen (and what @PietrH linked), is to alias/mask the method, and make the default S3 call the base implementation of the method. The problem with this, I think, is that packages that depend on frictionless-r would all have to re-import that S3 modified version of append in order for it to work properly. (See the tidyverse generics package for example)

My recommendation is to instead build an API around datapackage manipulations that provides pipe-able verbs with a common prefix for building / modifying datapackages, specifically for datapackages. So provide dpkg_append, or dpkg_mutate, etc., In addition to preserving the class, it would also give you the ability to validate the descriptor as modifications were made, if desired.

I think it's a big open question though what base verbs/operations would form an optimal expressive api here... If you ever have a brainstorming session, i'd definitely be interested in participating!

@peterdesmet peterdesmet modified the milestones: 1.2.0, 1.3.0 Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working complexity:high Likely complex to implement
Projects
None yet
Development

When branches are created from issues, their pull requests are automatically linked.

3 participants