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

Change which epi_archive operations have reference semantics #67

Closed
brookslogan opened this issue Apr 13, 2022 · 4 comments
Closed

Change which epi_archive operations have reference semantics #67

brookslogan opened this issue Apr 13, 2022 · 4 comments
Assignees
Labels
op-semantics Operational semantics; many potentially breaking changes here P0 high priority

Comments

@brookslogan
Copy link
Contributor

brookslogan commented Apr 13, 2022

Two potential approaches:

  • change epi_archive from an R6 class into an S3-classed-list containing a lazy_dt (from dtplyr)
  • rename current epi_archive R6 class to EpiArchive, make epi_archive an S3 list wrapper on top of EpiArchive that somehow prevents reference semantics from messing with users

Main complication:

  • We may eventually want disk-backed and/or updating epi_archives for delphi-epidata, WayBack Machine, GitHub, and/or other types of data sources; if these are also considered epi_archives rather than their own separate class, we want to ensure that they will have a sensible and compatible non-reference-semantics or side-effect-based interface

Other work:

  • Update docs and vignettes talking about R6 and reference semantics.
@brookslogan brookslogan added the P2 low priority label Apr 13, 2022
@brookslogan
Copy link
Contributor Author

The priority of this issue might be upgraded depending on feelings on #64.

@brookslogan
Copy link
Contributor Author

We discussed an alternative approach to this:

  • archive$function is potentially effectful (and may often have invisible output)
  • archive %>% function_potentially_with_another_name is not effectful (often may just clone first) (and is does not often produce invisible output)
  • merge should not have side effects on the first argument as it does now

@ryantibs
Copy link
Member

@lcbrooks Yes I'm currently in favor of the alternative approach. And I added something to #64 that is also consistent with this.

So I think it currently only remains to change merge() and epix_merge() to make them abide by the new philosophy. To be clear, here's what I'm thinking:

  • x$merge(y) should merge x and y, and overwrite x with the result.
  • epix_merge(x, y) should merge x and y, and return new epi_archive with the result, whose data table is a NEW object. That is, x and y are completely unchanged (as are their underlying data tables).

Thoughts?

@dshemetov dshemetov added P0 high priority and removed P2 low priority labels Apr 18, 2022
@brookslogan brookslogan changed the title Change epi_archive to not have reference semantics Change what epi_archive operations have reference semantics May 9, 2022
@brookslogan brookslogan self-assigned this May 9, 2022
@brookslogan brookslogan changed the title Change what epi_archive operations have reference semantics Change which epi_archive operations have reference semantics May 10, 2022
@brookslogan brookslogan added the op-semantics Operational semantics; many potentially breaking changes here label May 31, 2022
@brookslogan
Copy link
Contributor Author

brookslogan commented Aug 1, 2022

I believe the "alternative approach" was completed for the currently available operations in #101. Any further discussion / revisiting is moved to #181.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
op-semantics Operational semantics; many potentially breaking changes here P0 high priority
Projects
None yet
Development

No branches or pull requests

3 participants