octolog - Githubissues

assignUser commented 2 years ago

Reviewers:

Submitting Author Name: Jacob Wujciak-Jens Submitting Author Github Handle: !--author1-->@assignUser@annakrystalli<!--end-editor-- Reviewers: @pat-s, @annakrystalli

Due date for @pat-s: 2022-03-30 Due date for @annakrystalli: 2022-06-07

Archive: TBD Version accepted: TBD Language: en

Paste the full DESCRIPTION file inside a code block below:

Package: octolog
Title: Better 'Github Action' Logging
Version: 0.1.1.9000
Authors@R: 
    person("Jacob", "Wujciak-Jens", , "jacob@wujciak.de", role = c("aut", "cre"),
           comment = c(ORCID = "0000-0002-7281-3989"))
Description: Provides an API for 'Github' <https://github.com> workflow
    commands and makes it possible to signal conditions from within R that
    will create annotations when used within 'Github Actions'
    <https://github.com/features/actions> but raise normal R conditions
    when used interactively.
License: MIT + file LICENSE
URL: https://github.com/assignUser/octolog,
    https://assignuser.github.io/octolog/
BugReports: https://github.com/assignUser/octolog/issues
Imports: 
    cli (>= 3.0.0),
    fs,
    glue,
    magrittr,
    openssl,
    rlang (>= 1.0.0),
    utils,
    withr
Suggests: 
    knitr,
    covr,
    mockery (>= 0.4.2.9000),
    testthat (>= 3.0.0),
    rmarkdown
Remotes: 
    r-lib/mockery
Config/testthat/edition: 3
Config/testthat/parallel: true
Encoding: UTF-8
Roxygen: list(markdown = TRUE)
RoxygenNote: 7.1.2
VignetteBuilder: knitr

Scope

Please indicate which category or categories from our package fit policies this package falls under: (Please check an appropriate box below. If you are unsure, we suggest you make a pre-submission inquiry.):
- [ ] data retrieval
- [ ] data extraction
- [ ] data munging
- [ ] data deposition
- [x] workflow automation
- [ ] version control
- [ ] citation management and bibliometrics
- [ ] scientific software wrappers
- [ ] field and lab reproducibility tools
- [ ] database software bindings
- [ ] geospatial data
- [ ] text analysis
Explain how and why the package falls under these categories (briefly, 1-2 sentences): Octolog aims to make it easier for R author's to use the full power of Github Actions as a CI service.
Who is the target audience and what are scientific applications of this package? Using CI is recognized as an important part in the development process of (scientific) R software and octlog aims to make this easier and more powerful.
Are there other R packages that accomplish the same thing? If so, how does yours differ or meet our criteria for best-in-category?

As far as I know there are none.

(If applicable) Does your package comply with our guidance around Ethics, Data Privacy and Human Subjects Research?
If you made a pre-submission inquiry, please paste the link to the corresponding issue, forum post, or other discussion, or @tag the editor you contacted. https://github.com/ropensci/software-review/issues/501
Explain reasons for any pkgcheck items which your package is unable to pass.

Technical checks

Confirm each of the following by checking the box.

[x] I have read the guide for authors and rOpenSci packaging guide.

This package:

[x] does not violate the Terms of Service of any service it interacts with.
[x] has a CRAN and OSI accepted license.
[x] contains a README with instructions for installing the development version.
[x] includes documentation with examples for all functions, created with roxygen2.
[x] contains a vignette with examples of its essential functions and uses.
[x] has a test suite.
[x] has continuous integration, including reporting of test coverage using services such as Travis CI, Coveralls and/or CodeCov.

Publication options

[x] Do you intend for this package to go on CRAN?
[ ] Do you intend for this package to go on Bioconductor?
[ ] Do you wish to submit an Applications Article about your package to Methods in Ecology and Evolution? If so:

MEE Options

- [ ] The package is novel and will be of interest to the broad readership of the journal. - [ ] The manuscript describing the package is no longer than 3000 words. - [ ] You intend to archive the code for the package in a long-term repository which meets the requirements of the journal (see [MEE's Policy on Publishing Code](http://besjournals.onlinelibrary.wiley.com/hub/journal/10.1111/(ISSN)2041-210X/journal-resources/policy-on-publishing-code.html)) - (*Scope: Do consider MEE's [Aims and Scope](http://besjournals.onlinelibrary.wiley.com/hub/journal/10.1111/(ISSN)2041-210X/aims-and-scope/read-full-aims-and-scope.html) for your manuscript. We make no guarantee that your manuscript will be within MEE scope.*) - (*Although not required, we strongly recommend having a full manuscript prepared when you submit here.*) - (*Please do not submit your package separately to Methods in Ecology and Evolution*)

Code of conduct

[x] I agree to abide by rOpenSci's Code of Conduct during the review process and in maintaining my package should it be accepted.

ropensci-review-bot commented 2 years ago

Thanks for submitting to rOpenSci, our editors and @ropensci-review-bot will reply soon. Type @ropensci-review-bot help for help.

ropensci-review-bot commented 2 years ago

:rocket:

Editor check started

:wave:

ropensci-review-bot commented 2 years ago

Checks for octolog (v0.1.1.9000)

git hash: 8c283f19

:heavy_check_mark: Package name is available
:heavy_check_mark: has a 'CITATION' file.
:heavy_check_mark: has a 'codemeta.json' file.
:heavy_check_mark: has a 'contributing' file.
:heavy_check_mark: uses 'roxygen2'.
:heavy_check_mark: 'DESCRIPTION' has a URL field.
:heavy_check_mark: 'DESCRIPTION' has a BugReports field.
:heavy_check_mark: Package has at least one HTML vignette
:heavy_check_mark: All functions have examples.
:heavy_check_mark: Package has continuous integration checks.
:heavy_check_mark: Package coverage is 94.6%.
:heavy_check_mark: R CMD check found no errors.
:heavy_check_mark: R CMD check found no warnings.

Package License: MIT + file LICENSE

1. Statistical Properties

This package features some noteworthy statistical properties which may need to be clarified by a handling editor prior to progressing.

Details of statistical properties (click to open)

The package has: - code in R (100% in 4 files) and - 1 authors - 1 vignette - no internal data file - 8 imported packages - 20 exported functions (median 8 lines of code) - 30 non-exported functions in R (median 9 lines of code) --- Statistical properties of package structure as distributional percentiles in relation to all current CRAN packages The following terminology is used: - `loc` = "Lines of Code" - `fn` = "function" - `exp`/`not_exp` = exported / not exported The final measure (`fn_call_network_size`) is the total number of calls between functions (in R), or more abstract relationships between code objects in other languages. Values are flagged as "noteworthy" when they lie in the upper or lower 5th percentile. |measure | value| percentile|noteworthy | |:------------------------|-----:|----------:|:----------| |files_R | 4| 28.3| | |files_vignettes | 1| 68.4| | |files_tests | 4| 79.0| | |loc_R | 242| 27.0| | |loc_vignettes | 26| 3.4|TRUE | |loc_tests | 185| 53.1| | |num_vignettes | 1| 64.8| | |n_fns_r | 50| 56.6| | |n_fns_r_exported | 20| 67.4| | |n_fns_r_not_exported | 30| 52.8| | |n_fns_per_file_r | 9| 84.2| | |num_params_per_fn | 2| 10.4| | |loc_per_fn_r | 9| 24.3| | |loc_per_fn_r_exp | 8| 18.6| | |loc_per_fn_r_not_exp | 9| 27.1| | |rel_whitespace_R | 26| 40.1| | |rel_whitespace_vignettes | 81| 14.2| | |rel_whitespace_tests | 21| 50.9| | |doclines_per_fn_exp | 25| 23.5| | |doclines_per_fn_not_exp | 0| 0.0|TRUE | |fn_call_network_size | 38| 60.5| | ---

1a. Network visualisation

Click to see the interactive network visualisation of calls between objects in package

2. `goodpractice` and other checks

Details of goodpractice and other checks (click to open)

#### 3a. Continuous Integration Badges [![R-CMD-check](https://github.com/assignUser/octolog/workflows/R-CMD-check/badge.svg)](https://github.com/assignUser/octolog/actions) [![pkgcheck.yaml](https://github.com/assignUser/octolog/actions/workflows/pkgcheck.yaml/badge.svg)](https://github.com/assignUser/octolog/actions) **GitHub Workflow Results** |name |conclusion |sha |date | |:--------------------------|:----------|:------|:----------| |Octolog example workflow |success |8c283f |2022-02-02 | |pages build and deployment |success |9d6c29 |2022-02-02 | |pkgcheck |success |8c283f |2022-02-02 | |pkgdown |success |8c283f |2022-02-02 | |R-CMD-check |success |8c283f |2022-02-02 | |test-coverage |success |8c283f |2022-02-02 | --- #### 3b. `goodpractice` results #### `R CMD check` with [rcmdcheck](https://r-lib.github.io/rcmdcheck/) rcmdcheck found no errors, warnings, or notes #### Test coverage with [covr](https://covr.r-lib.org/) Package coverage: 94.57 #### Cyclocomplexity with [cyclocomp](https://github.com/MangoTheCat/cyclocomp) No functions have cyclocomplexity >= 15 #### Static code analyses with [lintr](https://github.com/jimhester/lintr) [lintr](https://github.com/jimhester/lintr) found the following 17 potential issues: message | number of times --- | --- Avoid changing the working directory, or restore it in on.exit | 2 Lines should not be more than 80 characters. | 15

Package Versions

|package |version | |:--------|:---------| |pkgstats |0.0.3.88 | |pkgcheck |0.0.2.227 |

Editor-in-Chief Instructions:

This package is in top shape and may be passed on to a handling editor

jooolia commented 2 years ago

@ropensci-review-bot assign @annakrystalli as editor

ropensci-review-bot commented 2 years ago

I'm sorry human, I don't understand that. You can see what commands I support by typing:

@ropensci-review-bot help

jooolia commented 2 years ago

@ropensci-review-bot assign @annakrystalli as editor

ropensci-review-bot commented 2 years ago

Assigned! @annakrystalli is now the editor

assignUser commented 2 years ago

Thank you @jooolia for your work so far!

I just wanted to note that my test coverage as displayed by {pkgcheck} has seemingly dropped below 75% but this is due to an issue with {covr} and the way it handles expect_snapshot tests: https://github.com/r-lib/covr/issues/482 The actual coverage is >90%: https://app.codecov.io/gh/assignUser/octolog

Edit: looks like the issue only happens with running {pkgcheck}/{covr} locally and using the gh-action as the result reported by the review bot is accurate.

annakrystalli commented 2 years ago

Hello @assignUser and many thanks for your package submission! 👋

I'll be your editor and will start editor checks shortly and report back. 👍

assignUser commented 2 years ago

Hey @annakrystalli do you need any input from my side at this point to continue the submission?

annakrystalli commented 2 years ago

Editor checks:

[ ] Documentation: The package has sufficient documentation available online (README, pkgdown docs) to allow for an assessment of functionality and scope without installing the package. In particular,
- [ ] Is the case for the package well made?
- [ ] Is the reference index page clear (grouped by topic if necessary)?
- [ ] Are vignettes readable, sufficiently detailed and not just perfunctory?
[x] Fit: The package meets criteria for fit and overlap.
[x] Installation instructions: Are installation instructions clear enough for human users?
[x] Tests: If the package has some interactivity / HTTP / plot production etc. are the tests using state-of-the-art tooling?
[x] Contributing information: Is the documentation for contribution clear enough e.g. tokens for tests, playgrounds?
[x] License: The package has a CRAN or OSI accepted license.
[x] Project management: Are the issue and PR trackers in a good shape, e.g. are there outstanding bugs, is it clear when feature requests are meant to be tackled?

Editor comments

Hello @assignUser! and many apologies for the delayed response. I had been battling with my compilers since I upgraded my laptop and I finally managed to get them sorted and run all checks and tests locally!

The package looks in good shape. There are a couple of very minor issues flagged up below. My main comment would be regarding documentation. I feel there is a little too much linking to outside resources, or to examples that don't clearly walk the user through how package functionality is implemented but rather leave them to have to figure it out.

My personal suggestion (that would have helped me understand the package more easily) would be:

To include the Introduction to octolog (first) section on the Getting Started vignette in the README (so users can get a flavour of functionality from the README itself). See the README & Documentation sections in the rOpenSci dev guide for details, in particular this:

The README, the top-level package docs, vignettes, websites, etc., should all have enough information at the beginning to get a high-level overview of the package and the services/data it connects to, and provide navigation to other relevant pieces of documentation. This is to follow the principle of multiple points of entry i.e. to take into account the fact that any piece of documentation may be the first encounter the user has with the package and/or the tool/data it wraps.
When demonstrating package usage, I feel it would be better to use a more real world example of usage. Currently you are demonstrating code in a .github/workflows/R/conditions.R file, not a typical place a user would expect to run code from, and don't show explicitly how the code is run through GitHub Actions. While ok for a quick demo perhaps in a README, I feel you a deeper dive is required in the vignettes. In there, it would be more useful to show how the code would run in a small demo package, either as part of a function or a test for example.
To have a separate vignette for the workflow commands that includes examples.
You could even consider splitting of the security chapter to a separate vignette if you wanted.

Lastly, the link to the docs in the README returns 404.

Minor issues:

A few things flagged up by goodpractice::gp()


── GP octolog ─────────────────────────────────────────────────────────────────────────────

It is good practice to

  ✖ avoid long code lines, it is bad for readability. Also, many people
    prefer editor windows that are about 80 characters wide. Try make your lines
    shorter than 80 characters

    R/commands.R:4:1
    R/commands.R:12:1
    R/commands.R:37:1
    R/commands.R:47:1
    R/commands.R:90:1
    ... and 9 more lines

  ✖ avoid calling setwd(), it changes the global environment. If you
    need it, consider using on.exit() to restore the working directory.

    tests/testthat/test-util.R:49:16
    tests/testthat/test-util.R:51:3
───────────────────────────────────────────────────────────────────────────────────────────

I will get on looking for reviewers straight away. Let me know what you think about the documentation comments. We could also wait to get the opinions of the reviewers.

annakrystalli commented 2 years ago

@ropensci-review-bot assign @pat-s as reviewer

ropensci-review-bot commented 2 years ago

@pat-s added to the reviewers list. Review due date is 2022-03-30. Thanks @pat-s for accepting to review! Please refer to our reviewer guide.

ropensci-review-bot commented 2 years ago

@pat-s: If you haven't done so, please fill this form for us to update our reviewers records.

assignUser commented 2 years ago

@annakrystalli Thanks for the feedback. I'll start to think about how to better show case it but I would also be interested in the reviewers opinions :)

annakrystalli commented 2 years ago

@assignUser Sounds good to me!

pat-s commented 2 years ago

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

Briefly describe any working relationship you have (had) with the package authors.
[x] As the reviewer I confirm that there are no conflicts of interest for me to review this work (if you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

[x] A statement of need: clearly stating problems the software is designed to solve and its target audience in README
[x] Installation instructions: for the development version of package and any non-standard dependencies in README
[x] Vignette(s): demonstrating major functionality that runs successfully locally
[x] Function Documentation: for all exported functions
[ ] Examples: (that run successfully locally) for all exported functions
[x] Community guidelines: including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogenerated via Authors@R).

Functionality

[x] Installation: Installation succeeds as documented.
[ ] Functionality: Any functional claims of the software been confirmed.
[x] Performance: Any performance claims of the software been confirmed.
[x] Automated tests: Unit tests cover essential functions of the package and a reasonable range of inputs and conditions. All tests pass on the local machine.
[x] Packaging guidelines: The package conforms to the rOpenSci packaging guidelines.

Estimated hours spent reviewing: 2,5

[x] Should the author(s) deem it appropriate, I agree to be acknowledged as a package reviewer ("rev" role) in the package DESCRIPTION file.

Review Comments

General

Overall I think the package is a useful contribution and can be of good use use in CI/CD workflows. It simplifies the combined usage of local and CI/CD execution. I will also try it out in combination with {tic} to better highlight and format log outputs! In fact, I've played around with it a bit in the octolog branch in {tic} (see CI runs) and the condition signaling worked as expected!

Please finde some comments below of points which could possibly be improved/changed to make the package even better :)

I think the motivation and intro could profit from one or two sentences more highlighting the scope of the package and how {octolog} might improve the experience of a package author/analyst who works with GHA. I think the word "additionally" suggests that the condition signaling in R is just a "nice side effect" whereas it is actually one of the main points. To be clear: I think this sentence just needs a slight rewrite and the word "additionally" should probably be avoided to not "value down" this part.

I was wondering where the name {octolog} originates from? I couldn't find a statement about it - maybe this could be added? (I assume it originates from the "octo" being GH logo animal - but maybe I am wrong? :)

Dependencies

I know this usually is a hot debate with many different views but let me share mine :) I see that there is a strong focus on tidyverse packages (which is not a problem per se and tidyverse packages are of course well programmed packages), yet they usually bring in additional dependencies and one has to ask themselves if it's worth it - especially on the R package side (scripting/data analysis is a different use case). Please don't see this is a "blocker" or similar, just as a personal opinion or "food for thought" :)

I think the package could be substantially reduced WRT to package dependencies:

Often the magrittr pipe is only used in scripting/analysis and not within packages as it can be easily replaced
{rlang} is similar to {magrittr} state above, though I see that it might do some things "better" than base R checks etc. I see that mainly rlang::trace_back() is used and one call to rlang::exec(). I haven't yet made use of the former but wanted to ask which additional benefits the use of rlang::trace_back() brings compared to traceback() WRT to the package functionality?
I like to use {fs} in scripting but try to avoid in packages due to it's dependency on libuv. This is especially costly and (possibly annoying for unexperienced users) in CI as the system dependency first needs to be installed. This costs time during every run and I think the {fs} dependency could be easily avoided.

Avoiding all of the above would leave the package with {cli}, {glue} and {withr} as direct dependencies (in IMPORTS) which would be an improvement to me.

README

The initial example and screenshot is somewhat hard to understand for first time users. To make it fully clear I'd suggest to also add an example how the error msg would look like in an interactive example - maybe even side by side in a a table?
Installation: Given that {pak} is still quite unstable and non-standard WRT to package installation (and is a third-party package), I'd not use it as the "default" installation method. I'd suggest to use install.packages() and mention {pak} as an alternative. In addition, remotes:: should be used instead of devtools::
The following URL in the README leads to a 404: website
Footnote 2 states that GHA is "only free for public repos". It is also free for private repos but with a limited amount of usage (in mins)

Vignettes

Get Started

The vignette indicates that {octolog} only works with {cli} message output and not with normal message() or warning() calls. If so, this should be highlighted more prominently at the start of the vignette or in the README
The example on octo_abort() should maybe get it's own section. Especially because "This can be useful if you want a workflow to fail (e.g. to block a PR) by throwing an error but do not want to stop the R process." is one of the selling features of {octolog}
The "masking" section could profit from an additional R code block showcasing the R code directly as the screenshot is somewhat hard to read. Also it might be of interested to note why these two functions differ and which one should be used in which scenario.
The "stopping command parsing" section could profit from a toy example. I find it quite hard to image a scenario in which the describe command could be of good use. The last two sentences "This is not intended to protect against other R code. Due to R's metaprogramming capabilities it is not possible to protect the token within the same R session." are not fully clear to me in this context. I think this section could profit from more context and a clear example :)

Functions

https://jacob.wujciak.de/octolog/reference/enable_github_colors.html : Examples throw an error
Maybe group the pkgdown reference output in meaningful categories?
octo_add_path() : the description is unclear to me
octo_mask_value(): examples error
octo_start_group(): The example errors. I think this could be worth getting it's own section in a vignette?
octocat(): what is it's relation to the other cli-based functions?

Stylistic

Github -> GitHub
It would be great to somehow differentiate the package from normal text, either by formatting is as code or wrapping it in brackets such as {octolog}
Typos:
- README: powerful,free (whitespace)
- Get started vignette: using octo_stop_commands.Internally the function
Screenshots: Could possibly be of higher quality

annakrystalli commented 2 years ago

Thanks @pat-s for your review! @assignUser I'm still looking for a second reviewer so I suggest holding out responding or making changes (at least not in the main branch) until both reviews are complete.

assignUser commented 2 years ago

Thank you for your helpful review @pat-s ! I will implement/address your points after the second review as suggested by @annakrystalli

annakrystalli commented 2 years ago

Dear @assignUser, Many apologies for the delay in the review. I've had a lot of trouble finding a second reviewer (and in between have also moved countries!). I'm settled now though so to get things moving again and given I have no conflict of interest with the package, I'll go ahead an volunteer as a reviewer. ☺️

annakrystalli commented 2 years ago

@ropensci-review-bot assign @annakrystalli as reviewer

ropensci-review-bot commented 2 years ago

@annakrystalli added to the reviewers list. Review due date is 2022-06-07. Thanks @annakrystalli for accepting to review! Please refer to our reviewer guide.

ropensci-review-bot commented 2 years ago

@annakrystalli: If you haven't done so, please fill this form for us to update our reviewers records.

annakrystalli commented 2 years ago

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

Briefly describe any working relationship you have (had) with the package authors.
[x] As the reviewer I confirm that there are no conflicts of interest for me to review this work (if you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

[x] A statement of need: clearly stating problems the software is designed to solve and its target audience in README
[x] Installation instructions: for the development version of package and any non-standard dependencies in README
[x] Vignette(s): demonstrating major functionality that runs successfully locally
[x] Function Documentation: for all exported functions
[ ] Examples: (that run successfully locally) for all exported functions
[x] Community guidelines: including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogenerated via Authors@R).

Functionality

[x] Installation: Installation succeeds as documented.
[x] Functionality: Any functional claims of the software been confirmed.
[x] Performance: Any performance claims of the software been confirmed.
[x] Automated tests: Unit tests cover essential functions of the package and a reasonable range of inputs and conditions. All tests pass on the local machine.
[x] Packaging guidelines: The package conforms to the rOpenSci packaging guidelines.

Estimated hours spent reviewing: 4

[x] Should the author(s) deem it appropriate, I agree to be acknowledged as a package reviewer ("rev" role) in the package DESCRIPTION file.

Review Comments

Fistly many apologies for the delays in getting the package through the review process @assignUser. As for the package, given how popular GitHub Actions are becoming (and it's great to hear it works nicely for tic too), it's a nice and clean addition to packages in the R continuous integration space.

Thank you as well again @pat-s for your excellent review. I pretty much agree with all of @pat-s's comments, especially the one on streamlining dependencies. I think the suggestion is a smart one and second it.

So as not to repeat the same points, I focus on a few additional comments I have, primarily building on my initial points in the editor's checks as well as some mentioned by @pat-s regarding documentation.

Documentation:

Minor points

In the pgkdown docs, there are many errors in examples of functions with the error could not find function <function_name>) e.g. enable_github_colors, encode_string, octo_warn and more (some of which run locally).
When running examples locally, often Sys.setenv(GITHUB_ACTIONS = "true") needed to be run. It would be good if example code cleaned up after itself with withr::local_envvar(GITHUB_ACTIONS = "true") like you do in tests.

General package documentation

While I appreciate that you point to a live example of GA runs using octolog from the README / Getting started, I feel more explanation of what is going on would be appreciated by new users.

For example, for starters, I would explicitly link to the test-octolog.yaml workflow that is being run.

Then I would point the user to the lines in the workflow that are running the R code that are generating the messages by focusing on this section and explaining what is going on: https://github.com/assignUser/octolog/blob/8e71de9077f0f6a3e5fbefb78be5c6f32c0cbf47/.github/workflows/test-octolog.yaml#L34-L89

More specifically I might explain that it's sourcing /.github/workflows/R/conditions.R (which is so short I might even include it in the docs) and then running R code from the workflow yaml itself.

For me the key is to make clear not just the code that generates the output in GA, but also where they appear in R code in the repo and how they then get run through GitHub Actions

Two additional suggestions to make the usefulness of the package to the R user more concrete and easy to experiment with.

While your examples in test-octolog.yaml are comprehensive with respect to the generic types of outputs the octolog API and your package offer, I feel they could be improved in terms of helping R users visualise why it would be useful to include them. It would be good to provide some more real life examples (especially more realistic R continuous integration scenarios) and explain some of the ways in which R code might be run in GA (e.g. raw R code in the workflow yaml, sourcing R code, code being run as part of package tests, package loading).

Perhaps you could set up a tiny R package and placing octolog code in actual function files in R. Messages could be deliberate error messages, package loading messages, package dependency checks, progress messages, basically whatever you think you might inspire users to visualise their use.

I don't want to be prescriptive here, I'm just trying to motivate some ideas. The important point of all of this is to give a better and broader view of how they might be useful in GitHub Actions to an R user.
If you set up such a demo repository, you could even set it up as a template that users could copy (or if not just fork) and direct them to use it as a playground for testing out package functionality. This could get them started much quicker with experimenting with package functionality and for me greatly improve understanding of how the package works and how they might use it.

Minor points

good practice notes

There are still a couple of suggestions being picked up by goodpractice::gp(). I feel the ones about line length are not a huge deal but they still remain best practice for readability.

The note about using setwd(), again, I'm not sure it's a big deal because it's being used in tests and it seems and you clean up nicely with withr::defer(setwd(old_wd)). I'm guessing this is the best way to achieve what you're trying to do. If there's no other way, I think this will be fine.

spell check

Probably the only potentially valid typo is in the roxygen docs for enable_github_colors:

sideeffects         enable_github_colors.Rd:29

Conclusion

I know it might feel like my suggestions are overboard but I feel it's really important to consider the perspective of complete beginners, walk them through very explicitly how things work (instead of linking to examples with quite a few working parts that they need to reverse engineer) and help them visualise how the package can help them.

Very open to hearing your thoughts on this!

annakrystalli commented 2 years ago

@ropensci-review-bot submit review https://github.com/ropensci/software-review/issues/502#issuecomment-1067080447 time 2.5

ropensci-review-bot commented 2 years ago

Logged review for pat-s (hours: 2.5)

annakrystalli commented 2 years ago

@ropensci-review-bot https://github.com/ropensci/software-review/issues/502#issuecomment-1149620624 time 4

ropensci-review-bot commented 2 years ago

I'm sorry human, I don't understand that. You can see what commands I support by typing:

@ropensci-review-bot help

annakrystalli commented 2 years ago

@ropensci-review-bot submit review https://github.com/ropensci/software-review/issues/502#issuecomment-1149620624 time 4

ropensci-review-bot commented 2 years ago

Logged review for annakrystalli (hours: 4)

assignUser commented 2 years ago

Thanks again to both of your for the informative reviews! It has been sometime but I managed to carve out some time to work on {octolog}. On that topic, @annakrystalli is there a deadline for me?

@pat-s I have removed {fs} and {magrittr}. {rlang} has to stay as it is a (hidden) dependency for cli_abort etc.. This also means that my actual dependencies are {cli} and {withr} with {glue} and {rlang} being "free" via {cli}. With these dependencies being very lightweight, without system dependencies and made for use in packages I don't think it is worth it to remove anymore dependencies (which would basically mean re-writing in base R 🙀 ).

While I am not entirely sure that {fs} is really such an issue (the compilation takes < 30s / use rspm & cache in CI!) it did not require big changes. {magrittr} on the other hand was completely superfluous and just added because I like how the code looks using it 😁.

@annakrystalli I removed most of the {lintr} issues but have too keep some of the line length ones as those are links and I have not found a way to add a line break without breaking the link.

With the "technical" issues addressed I will work on improving the documentation next!

annakrystalli commented 2 years ago

Nice work @assignUser! While we suggest a response within a couple of weeks of reviews being submitted, there isn't a hard deadline really to when all comments must be resolved. The work required to resolve comments varies from package to package and we all understand having to carve out time can be hard especially during summer holiday time! So don't worry too much about a given deadline. Look forward to seeing the documentation! 😃

pat-s commented 2 years ago

While I am not entirely sure that {fs} is really such an issue (the compilation takes < 30s / use rspm & cache in CI!) it did not require big changes. {magrittr} on the other hand was completely superfluous and just added because I like how the code looks using it 😁.

I also like to use both in scripting, though pkg dev is usually a different field. It's also not only about installation time/effort on CI but also on various systems (a system dep can be a blocker for some people in some restricted environments as they cannot install anything themselves - as a system engineer/admin I face this situation quite often). This reasoning does not really apply for your package as it is aimed at CI in the first place, but I'd would just call it "good practice" to think about these things and try to avoid such "convenience" packages in pkg dev if there are (robust) base R solutions - especially if the transition is cheap. Sometimes there are not or very complicated workarounds only, so it always differs...

(sorry for the wall of text, just some personal thoughts, it's ofc up to you what you do WRT to deps in the end :) )

maurolepore commented 1 year ago

Dear @assignUser, my EiC rotation just started and I'm checking the status of every open issue. Any updates?

assignUser commented 1 year ago

@maurolepore Hey! Sorry for the long silence, there are no updates but I will try to move this forward soon(tm)! Thank you for checking in.

annakrystalli commented 1 year ago

Hello @assignUser ! Was wondering whether you had any updates on progress on the package? Do you feel we should put the review on hold?

assignUser commented 1 year ago

Hey, sorry. I have done some work on this but github has changed the API in parts for security reason and I have to adapt to this (though it should not be to hard) I will update you in a bit (~2 weeks ok?)

jhollist commented 9 months ago

@assignUser I am currently serving as the EIC and wanted to check in on this submission. Have you had a chance to update the package?

assignUser commented 7 months ago

I really hoped I would have more time to get this up to speed but at the moment that just hasn't worked out. I do appreciate the work everyone has put in so far, of course especially @pat-s and @annakrystalli as reviewers. You suggestions are very good and gave a good perspective and I hope to implement them at some point.

With my time constriants even if the package get's accepted it would be defacto abandond from the get go and I don't think that's the purpose of this process. For that reason I am going to close this.

Thank you again everyone ❤️

ropensci / software-review

octolog #502

Archive: TBD Version accepted: TBD Language: en

Scope

Technical checks

Publication options

Code of conduct

Checks for octolog (v0.1.1.9000)

1. Statistical Properties

1a. Network visualisation

2. `goodpractice` and other checks

Editor-in-Chief Instructions:

Editor checks:

[x] Project management: Are the issue and PR trackers in a good shape, e.g. are there outstanding bugs, is it clear when feature requests are meant to be tackled?

Editor comments

Minor issues:

Package Review

Documentation

Functionality

Review Comments

General

Dependencies

README

Vignettes

Get Started

Functions

Stylistic

Package Review

Documentation

Functionality

Review Comments

Documentation:

Minor points

General package documentation

Minor points

good practice notes

spell check

Conclusion

ropensci / software-review

octolog #502

Archive: TBD Version accepted: TBD Language: en

Scope

Technical checks

Publication options

Code of conduct

Checks for octolog (v0.1.1.9000)

1. Statistical Properties

1a. Network visualisation

2. goodpractice and other checks

Editor-in-Chief Instructions:

Editor checks:

[x] Project management: Are the issue and PR trackers in a good shape, e.g. are there outstanding bugs, is it clear when feature requests are meant to be tackled?

Editor comments

Minor issues:

Package Review

Documentation

Functionality

Review Comments

General

Dependencies

README

Vignettes

Get Started

Functions

Stylistic

Package Review

Documentation

Functionality

Review Comments

Documentation:

Minor points

General package documentation

Minor points

good practice notes

spell check

Conclusion

2. `goodpractice` and other checks