Skip to content

Conversation

kaneplusplus
Copy link
Contributor

Summary

This package is super useful. Thanks very much for all of your work.

I'm submitting a pull request that provides the suppression of the reproducibility reports. I'm finding the feature useful for two reasons. First, while the report is great to find problems outside the scope of the execution of individual code blocks, but it is not always needed when presenting to a non-technical audience. It can even be slightly distracting, especially when not all of the checks are passed. Second, I have a site that is triggering a check-failure because of global variables (data and env), which my Rmarkdown files don't create. I don't think it's a problem with either the execution of the code chunks or the construction of the documents, and it's not clear how to debug.

The changes do not alter the current behavior. The difference is that if

suppress_reports: true

appears in the _workflowr.yml file, then the report is not included in the output html. Testing is included.

Checklist

Details

@jdblischak
Copy link
Member

@kaneplusplus Thanks for the PR! I'll review it tomorrow.

@jdblischak
Copy link
Member

Second, I have a site that is triggering a check-failure because of global variables (data and env), which my Rmarkdown files don't create. I don't think it's a problem with either the execution of the code chunks or the construction of the documents, and it's not clear how to debug.

@kaneplusplus I was able to reproduce this strange behavior. Turns out it was a bug in callr 3.3.0, which was released to CRAN on July 4th. It polluted the global environment with the environments data and env. This was fixed earlier today in the development version of callr on GitHub: r-lib/callr@9f7665e

@kaneplusplus
Copy link
Contributor Author

Thanks very much for tracking this down! It was, in part, what motivated the pull request. I'll update.

I would still be keen to suppress the reports but if you want to discuss further or even not incorporate, it's OK. I'm happy to continue sending pull requests that may be helpful.

Copy link
Member

@jdblischak jdblischak left a comment

Choose a reason for hiding this comment

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

Sorry this took so long for me to review. I'm having difficulty troubleshooting some recent changes in the upstream dependencies (which is why the CI builds are currently a mess).

How about changing the name of the setting to suppress_report? This way it is clear that the user could also use it in the YAML header of a specific Rmd file to only disable the report in that file.

@jdblischak
Copy link
Member

Also, could you please add the default setting of FALSE to the function wflow_options():

https://github.com/jdblischak/workflowr/blob/b902bbf059247d425d558eaf5009a769ade97954/R/wflow_options.R#L13-L18

@jdblischak
Copy link
Member

@kaneplusplus Please let me know if you have any questions or issues for updating your PR. I fixed the failing tests on the main master branch, so you'll want to pull these changes to your fork. I also edited tests/testthat/test-report.R, so there may be a conflict. Please let me know if you run into any trouble with the Git stuff (i.e. I don't want Git issues to distract you from updating the PR).

@kaneplusplus
Copy link
Contributor Author

@jdblischak Sorry for the delay. Yes, I'm happy to make the changes. I'll update today.

@kaneplusplus
Copy link
Contributor Author

kaneplusplus commented Jul 25, 2019

I'm running behind on tasks and probably won't get to this by Sunday. Again, apologies for the delay.

@jdblischak
Copy link
Member

No worries! There's absolutely no hurry because this new feature isn't blocking the addition of any other feature.

@kaneplusplus
Copy link
Contributor Author

OK, the changes have been made. Thanks for the review.

@jdblischak
Copy link
Member

Thanks for making these changes!

Unfortunately I seemed to have confused GitHub. I rebased your commits onto the current master and force pushed these to the master branch of your fork. But now GitHub has closed this PR, and your commits are no longer on your fork.

https://github.com/presagia-analytics/workflowr/commits/master
master...presagia-analytics:master

I'll fix this in a second, but first I'm going to investigate why GitHub failed.

@jdblischak
Copy link
Member

@kaneplusplus Could you please check two things for me?

  1. On the right side of the Pull Request you should see a box that says "Allow edits from maintainers". Could you please confirm that this box is checked?

image

  1. On your fork of workflowr, could you please go to Settings -> Branches and delete any "Branch protection rules"? The URL to this settings page should be https://github.com/presagia-analytics/workflowr/settings/branches

I apologize for this hiccup. And for your peace of mind, I've temporarily saved your commits in a branch. Once you make the changes to the settings above, I should be able to fix this on GitHub.

@jdblischak
Copy link
Member

Also, would you like to be added as an official contributor? If yes, please let me know if I should use your presagia.ai email or your yale.edu email. Thanks!

@kaneplusplus
Copy link
Contributor Author

OK, I think I've resubmitted. Please let me know if it's not sufficient. Happy to work through any issues with you.

@jdblischak
Copy link
Member

OK, I think I've resubmitted. Please let me know if it's not sufficient. Happy to work through any issues with you.

Since I borked the GitHub workflow, would it be ok if I closed the Pull Requests and just pushed your commits manually to master?

Also, would you like to be added as an official contributor? If yes, please let me know if I should use your presagia.ai email or your yale.edu email . Thanks!

Please let me know if you'd like to be a contributor and what contact info I should use.

@kaneplusplus
Copy link
Contributor Author

Sure. It's no problem at all.

I would love to be added as a contributor. I'm using workflowr a lot and I'll definitely have more updates in the coming weeks. My contact info is below.

  person(given = "Michael J.",
         family = "Kane",
         email = "michael.kane@yale.edu",
         role = c("ctb"),
         comment = c(ORCID = "http://orcid.org/0000-0003-1899-6662")))

@jdblischak
Copy link
Member

I would love to be added as a contributor.

Great! Thanks for your contribution.

I'm using workflowr a lot and I'll definitely have more updates in the coming weeks.

That's awesome! I look forward to your future updates, and I promise it'll be a smoother process next time :-)

Also, if you are going to be working on multiple different updates, I recommend implementing each on a separate feature branch, e.g. something like

cd workflowr/
git checkout -b feature-1
# make changes
git push origin feature-1
git checkout master
git checkout -b feature-2
# make changes
git push origin feature-2
git checkout master

This way you can make changes to each feature based on feedback without affecting the other Pull Requests.

@jdblischak
Copy link
Member

Also, since the master branch of your fork and the main master branch have gotten out of sync, it might be easiest for you to delete your current fork of workflowr and re-fork it on GitHub. This will ensure that your future Pull Requests will merge nicely.

@jdblischak
Copy link
Member

@kaneplusplus I added some docs and more tests, and also added you as a contributor.

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.

2 participants