search

etiennebacher / altdoc

Alternative to pkgdown to document R packages
https://altdoc.etiennebacher.com
Other
62 stars 9 forks source link

path destroy #235

Closed vincentarelbundock closed 8 months ago

vincentarelbundock commented 8 months ago

This is an illustration of what it could look like. The benefits I see are:

  1. Slightly simplification of the code: nice, but not a huge deal
  2. Easier to diagnose path bugs: maybe I'm just bad at this, but I often get confused with the current system.
  3. Most important: Constrains the user to use a feature set that we are confident in. I have had several bad experiences with path, and don't feel great about releasing this feature. But maybe it works well enough and i'm being paranoid...

There are test failures and merge conflicts, but I'd only finish those if you want to move ahead with this change.

vincentarelbundock commented 8 months ago

To be clear, it's totally fine if you don't want t merge this if you think path is useful. I don't want to force you hand.

etiennebacher commented 8 months ago

I'll have to think more about this. Originally I didn't have path but added it following https://github.com/etiennebacher/altdoc/issues/21. As in the issue, I still don't have a use case for this, except for CI and the rare cases where the root of a package is a subfolder (similarly I don't often see situations where the arg path is used in pkgdown). However I just (re-)discovered fs::path(), which would slightly simplify the code by using fs::path(path, "_quarto/docs") instead of fs::path_join(c(path, "_quarto/docs")) for example

Just for curiosity, I took a look at how pkgdown does this because they have the same issues. Basically they use as_pkgdown() at the beginning of their build_ functions and this creates an object that contains all the info they need: names of vignettes and man pages, source and destination paths, pkg name, pkg version, etc. Not saying we should do like them but it's interesting to look at it.

I have had several bad experiences with path, and don't feel great about releasing this feature. But maybe it works well enough and i'm being paranoid...

I'm curious about the issues you had, were they caused by a bug in the functions, by the fact that variable names are confusing, or by sth else?

In any case, maybe it works well but I don't think we test the behavior when path is not ".". So if we end up keeping this arg we should increase its coverage

vincentarelbundock commented 8 months ago

Cool cool. I like the pkgdown approach you describe.

I don't have a MWE right now but will let you know if there's a clear case.

vincentarelbundock commented 8 months ago

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...