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

"Promote" other_keys to be printed, a constructor parameter, and more clearly documented #186

Closed
brookslogan opened this issue Aug 5, 2022 · 4 comments · Fixed by #460 or #477
Closed
Labels
documentation Improvements or additions to documentation op-semantics Operational semantics; many potentially breaking changes here REPL Improved print, errors, etc.

Comments

@brookslogan
Copy link
Contributor

Migrated from #113 and #183.

  • print.epi_df should print the other_keys metadata.
  • other_keys should be its own parameter to new_epi_df and as_epi_df
  • Unfortunately, I don't think we ever explained what "keys" were in the context of an epi_df, and googling "tibble key" is surprisingly unhelpful (currently giving links to lock picks and decoder tools) [so the {new,as}_epi_df docs may need explication]. Maybe explaining this could just be rolled into the issue for adding other_keys as a parameter, though; it'd be easier to do in its own @param.

If this is causing recurring issues in epipredict maybe it should be changed to higher priority.

@brookslogan
Copy link
Contributor Author

brookslogan commented Aug 8, 2022

We should probably also make other_keys=character(0) the default. [Completed in #390.]

@brookslogan brookslogan added the op-semantics Operational semantics; many potentially breaking changes here label Sep 20, 2022
@brookslogan
Copy link
Contributor Author

And we should also print the rest of the epikey in epi_archive's print method as well (e.g., after the geo and time type).

@brookslogan
Copy link
Contributor Author

brookslogan commented Jan 26, 2024

We need to regenerate the saved data sets (here and in other repos) after this change, or maybe right now due to the other_keys default change, or add notes in implementation to try to maintain backwards compatibility with saved edf's, or...

@dsweber2
Copy link
Contributor

dsweber2 commented Jun 7, 2024

When "you say after this change", are you referring to the default of other_keys switching to character(0)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation op-semantics Operational semantics; many potentially breaking changes here REPL Improved print, errors, etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants