Skip to content

Conversation

@MichaelChirico
Copy link
Contributor

Closes #179

This also includes #178 since it's logically related and I noticed the former while working on this.

Since I'm working on a fork I don't know of a way to make the diff cleaner (usually I'd put the merge target as #178, but that would only work as a PR to my fork).

@MichaelChirico MichaelChirico changed the title Use data.table suvsetting to avoid copy of large objects Use data.table subsetting to avoid copy of large objects May 12, 2025
@flying-sheep
Copy link
Member

Hi, thanks! Does a S3 method have to be exported in order to exist? I’d prefer not to add a public API that’s unrelated to repr’s purpose!

@MichaelChirico
Copy link
Contributor Author

I guess it depends on what you mean by "exported" -- as you see in the NAMESPACE, we've registered an S3 method, so in some sense that's "public+exported".

OTOH:

  • partition_from_parts is not exported. I think that means downstreams also can't define their own S3 methods for this function, though I'm not sure (and not sure anyone would want that anyway)
  • repr::partition_from_parts, repr::partition_from_parts.data.table, etc., will not work, as these functions are not exported. ::: would be needed.

I think the approach I took here is natural; depending on your concerns, we could also fake S3 dispatch ourselves and keep everything private like:

partition_from_parts = function(...) {
  if (inherits(., "data.table")) return(partition_from_parts.data.table(...))
  partition_from_parts.default(...)
}

@flying-sheep
Copy link
Member

flying-sheep commented Jul 22, 2025

Ah, I see, there’s a separate export(...) for actually exported functions, sorry for the confusion!

Looks like R CMD check is complaining. Could you take care of that please?

@MichaelChirico
Copy link
Contributor Author

Yea, a very unfortunate feature of {roxygen2} that the same @export is used for S3 method registration as for public function export :\

@flying-sheep flying-sheep added this pull request to the merge queue Jul 23, 2025
@flying-sheep
Copy link
Member

Great, thank you!

Merged via the queue into IRkernel:main with commit be21096 Jul 23, 2025
2 checks passed
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 this pull request may close these issues.

repr_text does expensive copy of data.table input

2 participants