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

store session_info() as yaml #64

Open
wants to merge 2 commits into
base: sessioninfo
Choose a base branch
from
Open

store session_info() as yaml #64

wants to merge 2 commits into from

Conversation

ThierryO
Copy link
Contributor

@ThierryO ThierryO commented Sep 3, 2019

No description provided.

Copy link
Member

@florisvdh florisvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the YAML format suggestion @ThierryO. Using a standard avoids the introduction of a specifically tailored text format, while retaining the function's purpose.

As discussed already, I agree to explicitly mention the R-version for CRAN packages.

Regarding the packages part, some optimizations should be considered IMO (cf. the original implementation):

  1. the current YAML format introduces redundancy in the field names (loadedversion, date, source is repeated for each package).
    In the original version these field names were actually even omitted (the function's documentation filled this gap).
  2. an update of the R-version of CRAN packages will result in large diffs. While this effectively is a large change, I believe this can (and therefore, should) be kept more concise in the diff.
  3. package updates will be less easily interpreted just from a git diff: that is because the package names will generally not be present in the diff lines.

Some of these may be a matter of taste. I currently see no implementation alternative which avoids all previously mentioned cons and still uses a simple, single-file text format approach that is both easy on the eye and will generate nice git diffs.

Considering this example:

packages:
  assertable:
    loadedversion: 0.2.5
    date: '2019-05-07'
    source: CRAN (R 3.6.0)
  assertthat:
    loadedversion: 0.2.1
    date: '2019-03-21'
    source: CRAN (R 3.6.0)
  DBI:
    loadedversion: 1.0.0.9001
    date: '2019-07-31'
    source: Github (r-dbi/DBI@042efec)

When using a YAML approach, this might be an interesting alternative to meet the above aspects:

packages:
  CRAN (R 3.6.0):
    assertable: '0.2.5, 2019-05-07'
    assertthat: '0.2.1, 2019-03-21'
  Github (r-dbi/DBI@042efec):
    DBI: '1.0.0.9001, 2019-07-31'

sep = " ",
append = TRUE)

write_sessioninfo <- function(file, pkgs = NULL, include_base = FALSE) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you did not implement the extra arguments?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notice line 17: #' @inheritParams sessioninfo::session_info

R/write_sessioninfo.R Outdated Show resolved Hide resolved
Co-Authored-By: Floris Vanderhaeghe <[email protected]>
@ThierryO
Copy link
Contributor Author

ThierryO commented Sep 4, 2019

The current version makes it straightforward to read the sessioninfo data. The code below returns a dataframe with 3 variables and the package names as rownames.

si <- yaml::read_yaml("sessioninfo.yml")
do.call(rbind, si$packages)

Your suggestion is indeed more concise. However it will also require a function to read the sessioninfo back into R.

Feel free to modify to function to yield smaller yaml files. I don't have the time to do this.

@florisvdh
Copy link
Member

After some more discussion on this, I'm planning to implement the 'shrinked' YAML approach and add it here. As the accompanying reading function is not immediately needed, that will be postponed as an issue. A usecase for that may be comparisons on the environment within R, or approaches to set up an R environment. After that, we can also inform the sessioninfo developers about this work and they may pick it up.

@hansvancalster
Copy link
Contributor

@ThierryO and @florisvdh this PR looks almost ready. Can you take a look and see if anything remains to be done?

@florisvdh
Copy link
Member

@hansvancalster @ThierryO I have this (and the integration with #62, which this PR amends) since long on my todo list. Since renv became more in use, I did not consider it a priority, but the function is still useful to document scripts / projects which don't use renv.

With the considerations of earlier comments in mind, and insights from using renv, I would still like to revisit both PRs a bit more in depth, discuss/take decisions and implement them. I'd be happy to do that during our hackathon.

@hansvancalster
Copy link
Contributor

With the considerations of earlier comments in mind, and insights from using renv, I would still like to revisit both PRs a bit more in depth, discuss/take decisions and implement them. I'd be happy to do that during our hackathon.

Great! In that case I mention @inbo/bmk

@florisvdh florisvdh self-assigned this Mar 25, 2021
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.

3 participants