[v2] Add the toc function with the test failure. #155

JiaxiangBU · 2019-04-01T15:52:08Z

This follows the original pull request.
I add my original function in the dev branch and pull it into your dev branch, just test the git process you show me.
And I will follow your tips to edit the function in the two parts,

relative path
change the base logic of the function

s <- wflow_status()
rmd <- rownames(s$status)[s$status$published]
html <- to_html(rmd, outdir = s$docs)
html <- fs::path_file(html)

Just let you know, thank you for your help, I learn a lot from you.

jdblischak · 2019-04-01T16:52:02Z

I add my original function in the dev branch and pull it into your dev branch, just test the git process you show me.

@JiaxiangBU Great!

change the base logic of the function

This will be the biggest change. By using workflowr-specific functions, you should be able to write the function without adding any new dependencies. The only new dependency you should need to add is clipr.

Just let you know, thank you for your help, I learn a lot from you.

Of course! Thanks for helping improve workflowr!

Please let me know when you are ready for me to review the pull request.

JiaxiangBU · 2019-04-01T17:07:39Z

@jdblischak I follows your tips to use

to_html to get HTML paths
rmarkdown::yaml_front_matter to get titles
relative paths

and pull it, just let you know.

jdblischak

This is coming along great! Please see my comments and let me know what questions you have.

jdblischak · 2019-04-01T19:44:13Z

R/wflow_toc.R

+#' @import tibble
+#' @export
+
+wflow_toc <- function(analysis_dir = "analysis", docs_dir = "docs") {


The only argument the function needs is project = ".". wflow_status() returns the paths to these subdirectories.

My bad, I forget it, I will change it in the next commit.

I edit it, here, 8787eed

jdblischak · 2019-04-01T19:45:07Z

R/wflow_toc.R

+#' @export
+
+wflow_toc <- function(analysis_dir = "analysis", docs_dir = "docs") {
+  s <- wflow_status()


After adding project = "." as the default argument to wflow_toc(), you can change wflow_status() to wflow_status(project = project).

LGTM, I will do it.

I edit it, here, 8787eed

jdblischak · 2019-04-01T19:46:56Z

R/wflow_toc.R

+wflow_toc <- function(analysis_dir = "analysis", docs_dir = "docs") {
+  s <- wflow_status()
+  s$status %>%
+    tibble::rownames_to_column(var = 'rmd_path') %>%


Do you know how to perform these steps without relying on the tidyverse functions? My package doesn't currently use these packages, and I don't want to add a lot of new dependencies for just this one function. If needed, I can translate these to base R commands.

Yes, I agree with you. I will try it first.

I edit it, but I don't find a good substitute for dplyr::*join function, do you have any recommendations?
here is my handle, cdbb9c3

jdblischak · 2019-04-01T19:52:15Z

R/wflow_toc.R

+      link = glue::glue("1. [{name}]({html_path})")
+    ) %>%
+    .$link %>%
+    clipr::write_clip()


I'd like to make clipr a suggested package. If the user doesn't have clipr installed, then the function can print the results to the R console. Either way it would be good to return the character vector invisibly using return(invisible(x)), where x would be replaced with the character vector containing the toc.

You can check if a package is installed using the following strategy:

if requireNamespace("clipr", quietly = TRUE) { # Write to clipboard } else { # Print to R console }

I use your recommendation, but I do it in my way because I still get the point of the function invisible.
In my understanding, if the users have already installed the package clipr, they will give the toc in their clipboard. If not, the function wflow_toc will print the toc in the console?

Do I miss something?

…till relys on dplyr::inner_join

git status

JiaxiangBU · 2019-04-04T03:58:15Z

Hi @jdblischak , I think I finish most of them, and I am still working on some minimal parts. Now I am trying to update the DESC manually, do you know to have to update it automatically?
In our conversation, we will delete most of the dependencies from the package tidyverse.

JiaxiangBU · 2019-04-04T08:59:16Z

Hi @jdblischak , I think the rmarkdown chunk option results='asis' is better idea.

when set to 'asis', text output is written “as-is”, e.g., you can write out raw Markdown text from R code (like cat('**Markdown** is cool.\n'))

But I don't find a way to set this argument inside the function. Do you know something about it?
Here is some discussion on SO

jdblischak · 2019-04-04T11:03:35Z

Now I am trying to update the DESC manually, do you know to have to update it automatically?

@JiaxiangBU To update DESCRIPTION automatically, first delete the @import statements from the roxygen2 documentation and then run devtools::document().

JiaxiangBU · 2019-04-04T11:53:36Z

Now I am trying to update the DESC manually, do you know to have to update it automatically?

@JiaxiangBU To update DESCRIPTION automatically, first delete the @import statements from the roxygen2 documentation and then run devtools::document().

Hi, @jdblischak I do it and drop most of the tidyverse related packages.

jdblischak · 2019-04-04T15:32:59Z

@JiaxiangBU My mistake. document() only automatically updates NAMESPACE. You'll need to manually edit DESCRIPTION to remove the packages. When you do that, please add back the version specifications for fs and rmarkdown.

jdblischak

This is coming along great!

jdblischak · 2019-04-04T15:33:29Z

R/wflow_toc.R

+  s <- wflow_status(project = ".")
+  df<- s$status
+  df$rmd_path <- rownames(df)
+  df <- df[df$published]


To filter the rows, this needs to be df <- df[df$published, ]

Yes, I agree with you.

I make a new commit for this editing.

jdblischak · 2019-04-04T15:36:53Z

R/wflow_toc.R

+  df<- s$status
+  df$rmd_path <- rownames(df)
+  df <- df[df$published]
+  df$html_path <- to_html(df$rmd_path,outdir = s$docs) %>% basename()


Please don't use the pipe %>%. Instead, either break this into 2 lines:

df$html_path <- to_html(df$rmd_path,outdir = s$docs) df$html_path <- basename(df$html_path)

or nest the function calls:

df$html_path <- basename(to_html(df$rmd_path,outdir = s$docs))

jdblischak · 2019-04-04T16:00:32Z

R/wflow_toc.R

+  df$rmd_path <- rownames(df)
+  df <- df[df$published]
+  df$html_path <- to_html(df$rmd_path,outdir = s$docs) %>% basename()
+  file_name <- sapply(df$rmd_path, function(x) rmarkdown::yaml_front_matter(x)$title) %>%


This part is tricky since an Rmd file may not have a title defined. I think it may be best handled with a custom function. Can I send push a commit to your pull request to add this? Then you would get the change locally by running git pull.

Sure, that is great, I am looking forward to it.

I try a pull request from your dev branch to mine and now I know how it works.

jdblischak · 2019-04-04T16:04:34Z

R/wflow_toc.R

+  df$link <- paste0("1. [",df$name,"](",df$html_path,")")
+
+  # output
+  # if (requireNamespace("clipr", quietly = TRUE)) {


To be able to view the output even though it is copied to the clipboard, you can return the value of df$link at the end of the function with return(invisible(df$link)).

> x <- wflow_toc() The table of content of your project is on the clipboard. > x [1] "1. [About](about.html)" "1. [Home](index.html)" "1. [License](license.html)"

This will also make it easier to write automated tests for wflow_toc().

I get your point, so the users expect to get an output in a vector.

I make a new commit to edit it.

Dev

JiaxiangBU · 2019-04-05T02:44:03Z

@JiaxiangBU My mistake. document() only automatically updates NAMESPACE. You'll need to manually edit DESCRIPTION to remove the packages. When you do that, please add back the version specifications for fs and rmarkdown.

@jdblischak
Yeah, I delete the tidyverse packages in the DESC document and put the clipr in the suggested field.

JiaxiangBU · 2019-04-08T05:25:58Z

Yes, currently I use it in the index.html pages for several workflowr projects.

@JiaxiangBU My question was about including index.html in the list of analyses.

For example, on your main page for learn_rmd, your list of analyses includes Home, About, and License. Since links to these pages are already available in the navigation bar, it is somewhat redundant to include them in the list of rmd files. We could by default have wflow_toc() ignore any rmd files that are included in the navigation bar (by checking analysis/_site.yml), and then provide a function argument to optionally include them. Does that make sense?

I get your point and agree with you. I am thinking about it. When I update the function, I will let you know,.

JiaxiangBU · 2019-04-08T05:28:27Z

Next topic, I find based on this function, the toc is updated manually by users every time. Maybe an automatic solution is better.

JiaxiangBU · 2019-04-08T05:52:03Z

Hi John. I make a new commit for this.

title and HTML are extracted from this object, rmd, so I edit it first.
And I find the imported packages in the function document is deleted, so I don't add here package.

jdblischak · 2019-04-08T17:01:50Z

I make a new commit for this.

@JiaxiangBU Awesome! Good job parsing the YAML file!

title and HTML are extracted from this object, rmd, so I edit it first.

I agree that you want to filter the results early.

And I find the imported packages in the function document is deleted, so I don't add here package.

You don't need to use the here package. wflow_status() returns the path to the analysis directory (note that it doesn't need to called analysis/; a user can change it and wflow_status() will still identify the correct subdirectory).

I'll update the tests.

jdblischak · 2019-04-08T18:56:54Z

@JiaxiangBU I just pushed two new commits to your branch. Please pull them locally with git pull origin dev.

I changed the filtering to use html. This is because these are always just the filenames like in the navigation bar. The rmd vector may have other parts of the file path included, which is why it won't always work (we have to preserve the path to be able to read the title later on).

I also added some tests to catch some edge cases, e.g. when there is no navbar.

Next topic, I find based on this function, the toc is updated manually by users every time. Maybe an automatic solution is better.

Could you please elaborate on this idea? What would happen automatically?

JiaxiangBU · 2019-04-09T02:47:55Z

You don't need to use the here package. wflow_status() returns the path to the analysis directory (note that it doesn't need to called analysis/; a user can change it and wflow_status() will still identify the correct subdirectory).

Thanks, I have a deep understanding of this functon wflow_status().

JiaxiangBU · 2019-04-09T02:48:57Z

I changed the filtering to use html. This is because these are always just the filenames like in the navigation bar. The rmd vector may have other parts of the file path included, which is why it won't always work (we have to preserve the path to be able to read the title later on).

I think this change makes this function better, thanks.

JiaxiangBU · 2019-04-09T02:52:29Z

Next topic, I find based on this function, the toc is updated manually by users every time. Maybe an automatic solution is better.

Could you please elaborate on this idea? What would happen automatically?

For exmaple, in one of my workflowr project, I add a analysis document by wflow_open. And then I feel good, so use wflow_build, commit this adding, wflow_publish. I learn these steps from the tutorial you write.

However, in this processs, the toc I think is not updated. the new analysis document is not listed in the toc. Does it make sense to automatically add it in the toc?

Dev

JiaxiangBU · 2019-04-09T02:56:12Z

@JiaxiangBU I just pushed two new commits to your branch. Please pull them locally with git pull origin dev.

John, I update this package locally by git pull origin dev and on the GitHub by a new pull request. Thanks for your tips.

jdblischak · 2019-04-09T17:06:55Z

However, in this processs, the toc I think is not updated. the new analysis document is not listed in the toc. Does it make sense to automatically add it in the toc?

@JiaxiangBU Thanks for the explanation. I see how this would be convenient. Unfortunately this would be currently different to implement (i.e. to automatically re-run index.Rmd and insert an updated table of contents).

As a compromise, one option would be to add the following code chunk to analysis/index.Rmd:

```{r toc, results="asis", echo=FALSE}
suppressMessages(toc <- workflowr::wflow_toc())
cat(toc, sep = "\n")
```

This way, every time you built/published index.Rmd, it would insert an updated version of the table of contents.

Note: In general running workflowr functions from within an Rmd file is a bad idea, but this works for wflow_toc().

JiaxiangBU · 2019-04-10T03:57:56Z

suppressMessages(toc <- workflowr::wflow_toc())
cat(toc, sep = "\n")

Thus, I think just print a message with the content you write for the users. Users could just paste it in their index.Rmd. I think it is an option for users.

Note: In general running workflowr functions from within an Rmd file is a bad idea, but this works for wflow_toc().

Could you please explain this idea? Is there any limitation on the workflowr functions. I am just curious.

jdblischak · 2019-04-10T12:54:12Z

Could you please explain this idea? Is there any limitation on the workflowr functions. I am just curious.

@JiaxiangBU The main workflowr functions like wflow_build()/wflow_publish()/wflow_status() all perform operations based on the current status of the R Markdown files in the workflowr project. Thus if you run a workflowr command from within an R Markdown file contained in a workflowr project, this can become circular. It will probably still execute, but long-term it is not a good idea for maintaining a workflowr project, and it will likely lead to problems. Does that make sense?

jdblischak · 2019-04-10T12:59:52Z

@JiaxiangBU I think this PR is ready to merge. Could you please add an entry to NEWS.md? Please reference this PR, your GitHub username, and the original Issue #151.

Here's an example of a past entry:

* Improve support for hosting on Shiny Server. Setting the option
`fig_path_ext: false` in `_workflowr.yml` removes the file extension from the
figure subdirectories, allowing them to be viewed on Shiny Server (implemented
by @Tutuchan, #119, #122)

Also, would you like to be included as an official contributor?

JiaxiangBU · 2019-04-11T03:00:56Z

Could you please explain this idea? Is there any limitation on the workflowr functions. I am just curious.

@JiaxiangBU The main workflowr functions like wflow_build()/wflow_publish()/wflow_status() all perform operations based on the current status of the R Markdown files in the workflowr project. Thus if you run a workflowr command from within an R Markdown file contained in a workflowr project, this can become circular. It will probably still execute, but long-term it is not a good idea for maintaining a workflowr project, and it will likely lead to problems. Does that make sense?

I get your point. Thanks for your explanation.

… issue number

codecov-io · 2019-04-11T03:13:50Z

Codecov Report

Merging #155 into dev will increase coverage by 0.2%.
The diff coverage is 88.23%.

@@            Coverage Diff            @@
##              dev     #155     +/-   ##
=========================================
+ Coverage   86.93%   87.14%   +0.2%     
=========================================
  Files          25       26      +1     
  Lines        2871     2863      -8     
=========================================
- Hits         2496     2495      -1     
+ Misses        375      368      -7

Impacted Files	Coverage Δ
R/wflow_toc.R	`88.23% <88.23%> (ø)`
R/git.R	`75.64% <0%> (-5.38%)`	⬇️
R/wflow_git_push.R	`49.53% <0%> (+4.61%)`	⬆️
R/wflow_git_pull.R	`61.46% <0%> (+4.8%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9345f74...7da9253. Read the comment docs.

JiaxiangBU · 2019-04-11T03:18:09Z

@JiaxiangBU I think this PR is ready to merge. Could you please add an entry to NEWS.md? Please reference this PR, your GitHub username, and the original Issue #151.

Here's an example of a past entry:
* Improve support for hosting on Shiny Server. Setting the option
`fig_path_ext: false` in `_workflowr.yml` removes the file extension from the
figure subdirectories, allowing them to be viewed on Shiny Server (implemented
by @Tutuchan, #119, #122)

Hi, John, I make several commits to update news. I find there are several changes from origin dev, when I use git pull origin dev, I commit them separately, 4a88cc7.

JiaxiangBU · 2019-04-11T03:19:02Z

Also, would you like to be included as an official contributor?

Yes, I do like it.

jdblischak · 2019-04-11T17:03:43Z

@JiaxiangBU I added you as a contributor in 54b2afb. The function wflow_toc() will be included in the next release. Thanks again for your contribution to workflowr!

Related to Issue #151 and PR #155. Sometimes the system keyboard is not available when I run the tests with devtools::test(). Since this is technically an interactive session, the tests fail.

Related to Issue #151 and PR #155.

JiaxiangBU added 2 commits April 1, 2019 23:37

add new function wflow_toc

c49e35d

Export wflow_toc().

60b47b4

JiaxiangBU mentioned this pull request Apr 1, 2019

Add the toc function with the test failure. #154

Closed

JiaxiangBU added 4 commits April 2, 2019 00:16

Use relative path for the function R/wflow_toc.R

d348c27

Use relative path for the function R/wflow_toc.R

4f18ce2

Use rmarkdown::yaml_front_matter to get title.

162564a

Use rmarkdown::yaml_front_matter to get title.

259aeeb

Use wflow_* functions to get html paths

1824482

add used packages

beebed9

jdblischak requested changes Apr 1, 2019

View reviewed changes

JiaxiangBU added 3 commits April 4, 2019 11:08

Change the input arguments, and use the recommended one project

8787eed

Use base package functions instead of tidyverse ones, but wflow_toc s…

cdbb9c3

…till relys on dplyr::inner_join

Make the function clipr::write_clip like a option."

496c4e4

git status

document the function wflow_toc.Rd

bb1d5df

update the used packages, delete the tidyverse related packages

0fba084

jdblischak requested changes Apr 4, 2019

View reviewed changes

JiaxiangBU and others added 2 commits April 5, 2019 10:39

Merge pull request #1 from jdblischak/dev

50f2343

Dev

delete tidyverse packages in the DSEC document.

99d0526

JiaxiangBU and others added 4 commits April 5, 2019 10:46

nest the function calls

658cb46

subset a data.frame by df[index,]

e800b6b

make the toc function to return a vector invisibly

54349e9

Return results as a vector to facilitate testing.

f3d029b

add optional argument to ignore doc already in the nav bar.

368fea5

jdblischak added 2 commits April 8, 2019 14:21

Test ignore_nav_bar argument of wflow_toc().

a85286f

Document new arg, remove pipe, and test navbar edge cases.

37438e4

Merge pull request #2 from jdblischak/dev

04c5d32

Dev

JiaxiangBU added 4 commits April 11, 2019 11:08

update NEWS.md for the function wflow_toc, include username, original…

d15877e

… issue number

remove a space line

f2203fa

merge changes from origin dev

4a88cc7

edit the news for the function wflow_toc

7da9253

jdblischak approved these changes Apr 11, 2019

View reviewed changes

jdblischak merged commit 7da9253 into workflowr:dev Apr 11, 2019

jdblischak added a commit that referenced this pull request Apr 12, 2019

Create RStudio addin for wflow_toc().

ad95c94

Related to Issue #151 and PR #155.

[v2] Add the toc function with the test failure. #155

[v2] Add the toc function with the test failure. #155

Conversation

JiaxiangBU commented Apr 1, 2019

jdblischak commented Apr 1, 2019

JiaxiangBU commented Apr 1, 2019

jdblischak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JiaxiangBU commented Apr 4, 2019

JiaxiangBU commented Apr 4, 2019

jdblischak commented Apr 4, 2019

JiaxiangBU commented Apr 4, 2019 • edited

jdblischak commented Apr 4, 2019

jdblischak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JiaxiangBU commented Apr 5, 2019

JiaxiangBU commented Apr 8, 2019

JiaxiangBU commented Apr 8, 2019

JiaxiangBU commented Apr 8, 2019

jdblischak commented Apr 8, 2019

jdblischak commented Apr 8, 2019

JiaxiangBU commented Apr 9, 2019

JiaxiangBU commented Apr 9, 2019

JiaxiangBU commented Apr 9, 2019

JiaxiangBU commented Apr 9, 2019

jdblischak commented Apr 9, 2019

JiaxiangBU commented Apr 10, 2019 • edited

jdblischak commented Apr 10, 2019

jdblischak commented Apr 10, 2019

JiaxiangBU commented Apr 11, 2019

codecov-io commented Apr 11, 2019

Codecov Report

JiaxiangBU commented Apr 11, 2019

JiaxiangBU commented Apr 11, 2019

jdblischak commented Apr 11, 2019

JiaxiangBU commented Apr 4, 2019 •

edited

JiaxiangBU commented Apr 10, 2019 •

edited