335 remove aliases from package index

[chlebowa]

Fixes #335

Package index is built during the installation process but only once the package has been actually installed. A file is created, <package>/html/00Index.html, that is the source for the index page.

Here, a function is added that modifies html/00Index.html to remove HTML elements that link to a specified topic through aliases. The function is called in the loading hook.

@keywords internal is removed from all functions that are included in teal_slice, teal_slices, and filter_state_api.

[github-actions[bot]]

Code Coverage Summary

Filename                        Stmts    Miss  Cover    Missing
----------------------------  -------  ------  -------  ------------------------------------------------------------------------------------------------------------------------------------------------------
R/calls_combine_by.R                7       0  100.00%
R/choices_labeled.R                49      14  71.43%   25, 36, 41, 51-56, 68, 72-76
R/count_labels.R                   97       0  100.00%
R/filter_panel_api.R               29       1  96.55%   132
R/FilteredData-utils.R             68      25  63.24%   21-24, 27-30, 52-57, 153, 175-184
R/FilteredData.R                  562     227  59.61%   110, 184, 326, 398, 501-510, 533, 554-595, 613-616, 632, 673-706, 721-723, 727-733, 762-790, 813-815, 819-821, 824-838, 842-852, 855-898, 939, 962-984
R/FilteredDataset-utils.R          23       1  95.65%   125
R/FilteredDataset.R               170      61  64.12%   52, 152, 191, 216-273, 312-314
R/FilteredDatasetDataframe.R      121       8  93.39%   87, 148, 158, 234-238
R/FilteredDatasetDefault.R         18       4  77.78%   103-116
R/FilteredDatasetMAE.R            134      37  72.39%   56, 117-122, 161-166, 170-171, 189-211
R/FilterPanelAPI.R                 10       0  100.00%
R/FilterState-utils.R             101       2  98.02%   264, 294
R/FilterState.R                   361      61  83.10%   89, 212, 230-234, 241-242, 256-257, 263-264, 311, 313, 315, 367, 411, 639, 682-705, 715-734, 769-775, 784-790
R/FilterStateChoices.R            341     108  68.33%   310-313, 325, 367, 389-396, 400-417, 445, 458-469, 481-489, 493-522, 543-546, 549-552, 563-586, 597, 602, 613
R/FilterStateDate.R               212     129  39.15%   227, 279-436
R/FilterStateDatettime.R          309     199  35.60%   266, 318-549
R/FilterStateEmpty.R               53      31  41.51%   89, 99-104, 117, 129-169
R/FilterStateExpr.R                75      62  17.33%   149-272
R/FilterStateLogical.R            196     144  26.53%   136, 158, 218, 222-406
R/FilterStateRange.R              408     105  74.26%   262, 384, 510-514, 517-527, 530, 542-548, 559-571, 575-585, 589-591, 605-632, 647, 650, 664-681, 716-721, 731-733
R/FilterStates-utils.R             70       9  87.14%   108, 127, 188-194, 216, 245
R/FilterStates.R                  364      30  91.76%   78-82, 191, 314-323, 411-414, 457, 542-546, 591, 712-715
R/FilterStatesDF.R                  5       0  100.00%
R/FilterStatesMAE.R                10       1  90.00%   40
R/FilterStatesMatrix.R              3       0  100.00%
R/FilterStatesSE.R                211     157  25.59%   36, 71-73, 83-85, 109-116, 124-131, 154-302
R/include_css_js.R                  5       5  0.00%    12-16
R/remove_aliases.R                 20      20  0.00%    12-39
R/teal_slice.R                    107       4  96.26%   134, 186-187, 197
R/teal_slices.R                    84       5  94.05%   142-147
R/test_utils.R                     21       0  100.00%
R/utils.R                          18       0  100.00%
R/variable_types.R                 15       8  46.67%   44-51
R/zzz.R                            17      17  0.00%    3-48
TOTAL                            4294    1475  65.65%

Diff against main

Filename              Stmts    Miss  Cover
------------------  -------  ------  --------
R/remove_aliases.R      +20     +20  +100.00%
R/zzz.R                  +1      +1  +100.00%
TOTAL                   +21     +21  -0.32%

Results for commit: bc81ccbeeb5b2d01300360842ceb3ef7cccce064

Minimum allowed coverage is 80%

:recycle: This comment has been updated with latest results

[pawelru]

What if 00Index.html file is not writable? Use case: package is installed in centrally managed directory on the server and users are having read-only access.

[chlebowa]

Then no changes are made

  if (identical(index_file, "") || !isTRUE(utils::file_test("-w", index_file))) {
    return(invisible(FALSE))
  }

[gogonzo]

I would like this solution if pkgdown used installed package index - I run pkgdown::build_site and it seems that pkgdown build site from the source including all aliases in the rendered site

[m7pr]

Maybe for pkgdown we need some custom code in pkgdown.yml to be executed so that the pkgdown-index gets cleaned the same way it's done for package index.

[pawelru]

Then no changes are made

I am afraid that this would make it working only for subset of users. To my surprise, centrally managed environments is quite popular in the pharma world - even internally we have a BEE environment managed like that.

Is this really the only way to address this problem? Can it be done with modification of roxygen tags? Maybe we can slightly change the approach of documentation to obey this issue? Just thinking loudly...

[chlebowa]

Check this out:

EDIT: my mistake

[m7pr]

I actually like the idea, if this plays with pkgdown as it also clears aliases for filter-panel-api that are also duplicated (4 times).

[chlebowa]

pkgdown provides a way to exclude a topic from the reference but raises an error if you do :unamused:

[chlebowa]

Is this really the only way to address this problem? Can it be done with modification of roxygen tags? Maybe we can slightly change the approach of documentation to obey this issue? Just thinking loudly...

Be my guest.

[m7pr]

@pawelru

Can it be done with modification of roxygen tags?

It's a bigger problem and we researched multiple approaches. You can read https://github.com/insightsengineering/teal.slice/issues/335 and other PR https://github.com/insightsengineering/teal.slice/pull/565

[pawelru]

Can it be done with modification of roxygen tags?

It's a bigger problem and we researched multiple approaches. You can read #335 and other PR #565

Is this really the only way to address this problem? Can it be done with modification of roxygen tags? Maybe we can slightly change the approach of documentation to obey this issue? Just thinking loudly...

Be my guest.

I tried to avoid going deeper if possible. I need to focus on something else at the current moment. I can do that but a little bit later. Just wanted to flag this out for you in case you are unaware.

[chlebowa]

pkgdown provides a way to exclude a topic from the reference but raises an error if you do 😒

This seems to be a bug that was supposedly fixed in 2022. I will raise the issue again but even if they do fix it, I don't think they will do it soon.

[chlebowa]

I tried to avoid going deeper if possible. I need to focus on something else at the current moment. I can do that but a little bit later. Just wanted to flag this out for you in case you are unaware.

We are four days in, join us when you can :wink:

[m7pr]

pkgdown provides a way to exclude a topic from the reference but raises an error if you do 😒

This seems to be a bug that was supposedly fixed in 2022. I will raise the issue again but even if they do fix it, I don't think they will do it soon

Maybe we can run it with suppressWarnings/suppressMessages to avoid having it be displayed during GHA builds?

[chlebowa]

Maybe we can run it with suppressWarnings/suppressMessages to avoid having it be displayed during GHA builds?

-- Building function reference -------------------------------------------------
Error: 
! in callr subprocess.
Caused by error in `check_missing_topics(rows, pkg)`:
! All topics must be included in reference index
✖ Missing topics: filter_state_api
ℹ Either add to _pkgdown.yml or use @keywords internal
ℹ See `$stdout` for standard output.
Type .Last.error to see the more details.

Good luck.

[m7pr]

Got it, how do you run it? Did you add reference section to pkgdown.yml and run build_site or did you use build_reference?

[chlebowa]

[m7pr]

Looks like promising path!

[chlebowa]

@pawelru Re: file permissions, I forgot to mention that a package is test-loaded during installation and the index file is modified then.

[github-actions[bot]]

Unit Tests Summary

  1 files   29 suites   23s :stopwatch: 359 tests 359 :white_check_mark: 0 :zzz: 0 :x: 824 runs  824 :white_check_mark: 0 :zzz: 0 :x:

Results for commit bc81ccbe.

:recycle: This comment has been updated with latest results.

[pawelru]

@pawelru Re: file permissions, I forgot to mention that a package is test-loaded during installation and the index file is modified then.

That's great to hear. It means that my concern is not valid and I no longer see any blockers. However, I do encourage you to continue with looking for alternatives and do compare different approaches. I personally would prefer a fix to be incorporated in the already built package compared to apply fix on install. That's of course as long as it is possible.

Now I cannot say it's buggy or incorrect. It's just non-standard and I am afraid that I am just unable to assess all possible outcomes or edge-cases. Why do I care? Non-standard ways of working introduce some difficulty and requires some/deep knowledge and generally speaking i do want to have a low and cheap maintenance code. I really hope you know what I am mean. I did a very quick search on GH CRAN mirror: https://github.com/search?q=org%3Acran+path%3A*.R+00Index.html&type=code Out of 26k+ repos only 16 of them are using 00Index.html file and looking briefly most of them do read only. (It looks like only one knitr does write but I don't want to spend my time trying to understand it - that's not the point to get an exact number).

I know I am sitting on a very comfy reviewer position but I think that there is enough IQ points already there to came up with something good :) You are all clever folks and I believe in you 💪

[chlebowa]

Out of 26k+ repos only 16 of them are using 00Index.html file

False. The file is built by internal functions of the tools package upon installation. Every package that has a package index page (which I believe means virtually every package in existence) has that page. Why the page is not built with the INDEX file from the package root directory, which can be modified in the source package - I don't know.

I do encourage you to continue with looking for alternatives and do compare different approaches.

Believe me, I tried. This is the best I was able to come up with. See https://github.com/insightsengineering/teal.slice/pull/568 for an inferior alternative.

Now, is this bulletproof? I don't know. For example, my way of dropping rows from the HTML file is rather primitive and makes some assumptions. But here we are.

[pawelru]

Ahhh I knew you will find something in my statement :) I really tried my best! Let me correct myself. That statement should go as follows

Out of 26k+ packages on CRAN, only 16 are accessing 00Index.html file in its sources. Accessing for read / write or any other purpose. They all might have it (and use it) but only very small number of packages is doing actual modifications.

Anyway, that's just to show why I think it's non-standard. I will try to have a look at this but in the middle of next week the earliest.

[chlebowa]

Out of 26k+ packages on CRAN, only 16 are accessing 00Index.html file in its sources.

Fair :smiley: One of the reasons it is rarely accessed is it does not exist in the source. I agree, it is nonstandard.

[m7pr]

This is not urgent, let's wait and give @pawelru some time to explore. @chlebowa I'm amazed how far you went with your research. Totally impressed

[chlebowa]

I raised an issue in the pkgdown repo but no help will be coming.

This is what I have been able to accomplish with the current state of affairs:

I suggest we apply it as is.

[pawelru]

I just came across this: https://github.com/r-lib/xml2/pull/437

This package cannot be cross compiled, because it tries to evaluate something at install time. Therefore p3m and webr and r-universe need to patch this package, which is a headache

A topic of compilation for WASM binaries. We are not doing right now but I this is something that I really want to (still to be prioritised). Such non-standard fix-on-install type of activities might create a lot of issues down the way.

[chlebowa]

I don't think it is relevant.