`path` argument (again)

[vincentarelbundock]

Revisiting this, because I am having a lot of trouble with the path argument. Some of which include:

1. path doesn't work for me with a mkdocs virtual env.
2. tests break due to path issue in ways I can't diagnose when I make an ultra minor and seemingly completely unrelated change: wrapping sidebar creation in as.list() to fix the quarto yaml when there is only one vignette.
3. I had problems with docsify and path today, but can't even reproduce now.

Every time I try to diagnose and fix this, I just go through 4-deep nests and path passing, and I find the code super confusing. And the whole thing feels really fickle right now.

How about we remove the path argument, add a check for root dir at the very top of both functions, but leave the path code in for now. We can then revisit if we think this is actually a useful feature and worth the trouble.

Long term, if we decide it's not that useful, I can clean up the code to remove the extraneous stuff under the hood. If we decide it is useful, I think some kind of refactor will be necessary to clean things up.

How do you feel about all this?

[etiennebacher]

How about we remove the path argument, add a check for root dir at the very top of both functions, but leave the path code in for now.

What would happen to cases where the root directory doesn't correspond to the pkg directory like tinytest? For these cases we need to provide a path arg so that we can specify what is the pkg directory in the CI workflow.

I agree that path handling is annoying, and I feel there are a lot of repetitions in the code about this. This should probably be refactored but I'm still thinking of the best way to do so.

path doesn't work for me with a mkdocs virtual env.

This should now be fixed with the recent commits on main.

[vincentarelbundock]

Can we have a path variable and add some kind of setwd() in the GitHub action?

Repos like tinytest are pretty rare anyway, and GH is secondary funxtuonality. Once everything is rock solid on the primary stuff, this can be handled.

I'll take a look at the internals again and try to think of a solution.

I have some weird global settings in modelsummary where I store variables in a package specific environment. Maybe we can do something similar.

[etiennebacher]

I have some weird global settings in modelsummary where I store variables in a package specific environment. Maybe we can do something similar.

I was thinking about sth like this. We could have three "constants" SRC_DIR, SRC_DOCS and SRC_ALTDOC that point to the root, docs, and altdoc directories respectively and that are set when the user calls setup_docs(). The problem is that we would still need to use fs::path_join() all the time anyway so I'm not sure it would reduce the code complexity

[etiennebacher]

I also think we should harmonize variable names in functions (not only arg names but also object names inside the function). Currently I'm getting lost between src_dir, tar_dir, path, origin etc.

It is worth having slightly longer names to clarify what they represent

[vincentarelbundock]

I also think we should harmonize variable names in functions (not only arg names but also object names inside the function).

Totally agree. I can try this, but it might take me a bit of time.

The problem is that we would still need to use fs::path_join() all the time anyway so I'm not sure it would reduce the code complexity

Yeah. I think I'm leaning toward: It's ok to inconvenience the user a little bit to make developers happy (especially since we are the ones wasting our free time on this, and because there aren't a ton of users yet). This is easy enough: setwd("tinytest/pkg");altdoc::render_docs()

[etiennebacher]

Totally agree. I can try this, but it might take me a bit of time.

I'm currently refactoring several bits so we can delay this to a bit later

Regarding the paths, I'm still unsure how removing the arg path and using setwd() will help. Some things would be simplified, e.g

altdoc_dir <- fs::path_abs(fs::path_join(c(path, "altdoc")))
fs::path_join(c(altdoc_dir, file_name))

would become

fs::path_join(c("altdoc", file_name))

but in the end we still need some way to concatenate several bits of paths together, either with fs::path_join() or with paste0(). We could also do our own wrapper like:

make_path <- function(start, ...) {
  fs::path_abs(fs::path_join(c(...)), start = start)
}
# user input
path <- "pkg"

make_path(path, "altdoc", "freeze.rds")
#> C:/Users/etienne/AppData/Local/Temp/RtmpiwKiCc/reprex-57ecf555f34-stout-scaup/pkg/altdoc/freeze.rds

Bottom line, removing the path arg will also take some work for some unclear benefits. If you have something clearer in mind, do you want to open a draft PR where you modify a single file so we can discuss with more concrete examples?

[vincentarelbundock]

too many conflicts, and I have too little time to invest to make that work, so closing now.

If you don't want the next release to include a path argument, then we can just remove the argument in the external user interface, and call .check_is_package() at the top. Alternatively, tests for path would be important before release, but I'm not sure I'll have time to figure that out.

I'm basically feature complete in terms of what I wanted to accomplish here. Sorry this turned out to be so much work for you. I hope you think the package has improved, at least...

[etiennebacher]

I think we should keep the path argument. It's already there anyway and it seems weird to provide a lot of work to remove it, especially when the code simplification will be limited.

I think the package improved a lot, even if I spent more time on it that I originally planned, so thanks a lot for that. I'm in a weird situation now where I want to share this package around because I think some people may find it useful, but at the same time I don't want to spend too much time on maintenance. My priority is to have something that works well for polars and some other packages.

[vincentarelbundock]

FWIW, I do plan to help with maintenance, though not at the same speed..