teal_slice should show up in package index

[chlebowa]

The teal_slice help topic extensively documents the teal_slice and teal_slices classes and explains at length how filter states are defined and handled. Although all the functions in that topic are not exported, I believe teal_slice should be visible in the package index in RStudio's Help tab.

[donyunardi]

The documentation for these functions are robust and would be great if we can resurface this.

Acceptance Criteria

• Surface the documentation of teal_slice and teal_slices in index page.

[m7pr]

The current issue is that teal_slice and teal_slices are exported. However in their .R files we also host their utility functions like, as.teal_slice or print.teal_slices. The utility functions are marked with @keywords internal which removes them from the index, but their @rdname tag point to either teal_slice or teal_slices which removes those two from the index as well.

We need to find a solution that only add teal_slice and teal_slices to the index and also a way to have utility functions documented close to the main functions.

[m7pr]

Will investigate the usage of INDEX file from Writing R Extensions https://cran.r-project.org/doc/manuals/R-exts.html#The-INDEX-file

[m7pr]

The optional file INDEX contains a line for each sufficiently interesting object in the package, giving its name and a description (functions such as print methods not usually called explicitly might not be included). Normally this file is missing and the corresponding information is automatically generated from the documentation sources (using tools::Rdindex()) when installing from source.

[m7pr]

I think the INDEX file is a dead end. Even though I created INDEX manually using tools::Rdindex("man/",outFile="INDEX") and then edited the INDEX (added MARCIN to one of the titles) this didn't make an impact on the package index.

Even when I removed the package, build a source of the package and installed the package from the source

install.packages('../teal.slice_0.5.0.9004.tar.gz', repos = NULL, source = TRUE)

I can see that INDEX file contains custom text (MARCIN) in the R library

but the package index does not contain this change

[m7pr]

@chlebowa what would you say if we put @noRd tag for functions having @rdname teal_slice (and respectively @rdname teal_slices), which is for print, format, as.list, as and is and document their usage in a separate section of teal_slice / teal_slices documentation? Or we create a separate file teal_slice(s)-utilities that covers the documentation for utility functions?

[chlebowa]

is.teal_slice, as.teal_slice, as.list.teal_slice, and format.teal_slice must be on the same help page as teal_slice.

[m7pr]

We could keep them in the same page for teal_slice but remove them from usage, so they don't have single entries in index of the package. However, we could list them in a separate section of teal_slice documentation so they are on the same page. Not ideal, but trying to find a compromise.

[chlebowa]

We could keep them in the same page for teal_slice but remove them from usage, so they don't have single entries in index of the package.

🤨
Show me.

[m7pr]

@chlebowa so there is multiple options:

1.

We can extend description section with a sentence

#' Check out [teal_slice-utilities] functions for working with `teal_slice` object.

That would point to a separate documentation page

The reasoning for extending the description section is that it is visible on the top of teal_slice documentation page..

2.

The same as in 1. but we also list all utility functions in another sentence.

#' Check out [teal_slice-utilities] functions for working with `teal_slice` object.
#' Class is equipped with [`is.teal_slice`], [`as.teal_slice`], [`as.list_teal.slice`], [`format.teal_slice`] and
#' [`print.teal_slice`].

3.

We create a separate section in teal_slice documentation

#' @section Utility Functions:
#'
#' Check out [teal_slice-utilities] functions for working with `teal_slice` object.
#' Class is equipped with [`is.teal_slice`], [`as.teal_slice`], [`as.list_teal.slice`], [`format.teal_slice`] and
#' [`print.teal_slice`].

4.

We only extend See Also section

#' @seealso [`teal_slices`], [teal_slice-utilities] ([`is.teal_slice`], [`as.teal_slice`], [`as.list_teal.slice`], [`format.teal_slice`] and
#' [`print.teal_slice`]).

Separate documentation for utilities

Examples can either stay in teal_slice (for greater visibility of utility functions) or can be moved to the separate documentation page.

x1 <- teal_slice(
  dataname = "data",
  id = "Female adults",
  expr = "SEX == 'F' & AGE >= 18",
  title = "Female adults"
)
#' is.teal_slice(x1)
#' as.list(x1)
#' as.teal_slice(list(dataname = "a", varname = "var"))
#' format(x1)
#' format(x1, show_all = TRUE, trim_lines = FALSE)
#' print(x1)
#' print(x1, show_all = TRUE, trim_lines = FALSE)

The separate documentation page is documented like below:

#' Utility functions for `teal_slice` object
#' Convenience functions for working with `teal_slice` object.
#' @param x (`teal.slice`)
#' @param show_all (`logical(1)`) indicating whether to show all fields. If set to `FALSE`,
#'  only non-NULL elements will be printed.
#' @param trim_lines (`logical(1)`) indicating whether to trim lines when printing.
#' @param ... further argument passed to other methods.
#' @seealso [`teal_slice`]
#'
#' @rdname teal_slice-utilities
#' @export
#' @keywords internal
#'
is.teal_slice <- function(x) { # nolint
  inherits(x, "teal_slice")
}

#' @rdname teal_slice-utilities
#' @export
#' @keywords internal
#'
as.teal_slice <- function(x) { # nolint
  checkmate::assert_list(x, names = "named")
  do.call(teal_slice, x)
}

#' @rdname teal_slice-utilities
#' @export
#' @keywords internal
#'
as.list.teal_slice <- function(x, ...) {
  formal_args <- setdiff(names(formals(teal_slice)), "...")

  x <- if (isRunning()) {
    reactiveValuesToList(x)
  } else {
    isolate(reactiveValuesToList(x))
  }

  formal_args <- intersect(formal_args, names(x))
  extra_args <- rev(setdiff(names(x), formal_args))

  x[c(formal_args, extra_args)]
}


#' @rdname teal_slice-utilities
#' @export
#' @keywords internal
#'
format.teal_slice <- function(x, show_all = FALSE, trim_lines = TRUE, ...) {
  checkmate::assert_flag(show_all)
  checkmate::assert_flag(trim_lines)

  x_list <- as.list(x)
  if (!show_all) x_list <- Filter(Negate(is.null), x_list)

  jsonify(x_list, trim_lines)
}

#' @rdname teal_slice-utilities
#' @export
#' @keywords internal
#'
print.teal_slice <- function(x, ...) {
  cat(format(x, ...))
}

[chlebowa]

So they're not on the same page 😃

[m7pr]

There is a possibility to manually edit the .Rd file, where you put below in usage{}

is.teal_slice(x)

as.teal_slice(x)

\method{as.list}{teal_slice}(x, ...)

\method{format}{teal_slice}(x, show_all = FALSE, trim_lines = TRUE, ...)

\method{print}{teal_slice}(x, ...)

You can do the same for documenting parameters. Put below in arguments{}

\item{x}{(\code{teal.slice})}

\item{...}{further argument passed to other methods.}

\item{show_all}{(\code{logical(1)}) indicating whether to show all fields. If set to \code{FALSE},
only non-NULL elements will be printed.}

\item{trim_lines}{(\code{logical(1)}) indicating whether to trim lines when printing.}

[m7pr]

We can use @usage roxygen2 tag for this.

[chlebowa]

Can you try the @usage tag and display the results?

[m7pr]

Yes, I think I've got this. Will create PR

[m7pr]

Hey created a POC for teal_slice documentation in here #564 #565
If this is acceptable, will redo the same for teal_slices.

[m7pr]

Sorry. needed to rename the branch and created new PR #565

[donyunardi]

@chlebowa @m7pr
Thanks for looking into this.

After reviewing and observing all the workarounds that both of you had to implement in #565, I am impressed but also concerned about the maintenance aspect of this in the future.

Personally, I think we should just make the utility functions into a separate Rd file so that we can display only teal_slice and teal_slices in the index page.

Looking at @m7pr proposal, I prefer a combination of options 3 and 4.

I should add that these utilities functions are already shown in @examples for teal_slice and teal_slices. I think all of these should be enough hints.

[m7pr]

@averissimo I know you also spent some time yesterday on this. Any thoughts, or wishes, or blessings?

[averissimo]

Blessing to your latest suggestion!! 💯 You did an awesome job there and I agree with it

(#565 (comment)) My final idea was to (for each utility function) create a small documentation page

Yesterday I tried creating a mirror of the teal_slice(s) that had the same content using @inherit and another trick to minimize repetitive documentation, but I couldn't get it to generate the individual @params.

Having a small doc that directs to teal_slice(s) seems like the way to go.

ps. learned about TIL about @evalRd (yesterday actually and this r-lib/roxygen2#761 made me chuckle)

[chlebowa]

Wait, I think I'm on to something.

[m7pr]

@chlebowa I was not planning to push. I was actually thinking of us taking a step back and giving us some time to think about something different for the next 2-3 days :) ! Fingers crossed for your newest idea!

[m7pr]

@averissimo

It would be easier to document this if I remembered how it worked 😆

HAHA

[m7pr]

Summary

Hey gang, so a small summary of the current state (especially for @pawelru).

The goal

The issue that this card tried to solve was that ?teal_slice and ?teal_slices were not listed in package index because the functions included in the same manual page had @keywords internal roxygen2 tag. The goal of this card was to bring back ?teal_slice and ?teal_slices into package index, without the need to pollute the index with other helper functions listed on the same page as them (like S3 methods or utility functions - as.teal_slice, format.teal_slices, etc.).

What we tried

• Creation of custom INDEX file, which looks like is a dead functionality and does not influence package index
• Possibility to document utility functions on one manual page, and just mention them on teal_slice manual page
• Custom @usage section which allowed to create custom Usage section but required to put @aliases of all functions and methods used in the section, which lead to population of package index will all utility functions
• Modification of inst/Meta/*.Rd files, which occurred in R CMD Check Warning
• Modification of INDEX.html during the package installation where we run a script in zzz.R that during on-load removes @aliases from INDEX.html

[pawelru]

Let me ask you a quick question. Why do we have @keywords internal for exported (!) functionality? An example:

teal.slice/R/teal_slices.R

Lines 130 to 136 in 255283d

	#' @rdname teal_slices
	#' @export
	#' @keywords internal
	#'
	is.teal_slices <- function(x) { # nolint
	inherits(x, "teal_slices")
	}

If we remove it the problem is gone - am I right?

If it happens that there is a non-exported functionality - why to put it into the exported functionality manual? This supposed to be documented in another article - also the problem should be gone.

[chlebowa]

Because we want that function exported but not on the index.

[chlebowa]

Please don't make me say that @keywords internal doesn't mean a "private function" every week.

[m7pr]

@keywords internal it's only for handling functions that we don't want to have included in the index. They are often documented and exported. In this case there is a joint documentation. I think it is very smooth of what @chlebowa prepared so far

[pawelru]

Why do we don't want it to be in the index?

[chlebowa]

Because we don't want an index like this

[pawelru]

What's wrong with the above?

I have to admit that I personally see it as a lesser devil compared to the alternative of modifying 00Index.html file.

• we have anyway a much more cleaner reference page in pkgdown that we keep referencing to. I really doubt the index is frequently used.
• index is supposed to be an index - a list of elements. It is not meant to be read from the very beginning until the very end. This is a list of elements where users can search and find objects. From that perspective this is good.
• Last but not least - I am not super convinced to the alternative for the reasons I already mentioned there.

EDIT: Also, I am trying to workaround the problem as it seems that there are no good way of solving it.

[chlebowa]

• we have anyway a much more cleaner reference page in pkgdown that we keep referencing to. I really doubt the index is frequently used.
• index is supposed to be an index - a list of elements. It is not meant to be read from the very beginning until the very end. This is a list of elements where users can search and find objects. From that perspective this is good.

I beg to differ.

[pawelru]

I don't think we are the very first and only ones who discover this so let me find what others do:

• tibble - index consists of $.tbl_df, $<-.tbl_df, [.tbl_df, [<-.tbl_df (and other similar) functions
• data.table - index consists of [.data.table, is.data.table, is.na.data.table and other S3 methods like these

My point is - it looks like this is totally fine to put rd-linked methods as a separate entries in the index (i.e. not add keywords internal). It looks to be totally fine by the community - noone raise an issue about it.

You might have a different view so let's formulate a problem and challenge this from a different angle.

It is not supported to have an index listed topic that includes object with additional @keyword internal - let's just simply break that relation, i.e. an idea to create a separate man page for those non-indexed objects. This is also a common practice when I looked for how others are using @keywords internal. A typical use case is to include deprecated / superseded objects in the separate "xyz-superseded" man page (in particular: not include them in the new object that replaces it). In this context it could be "teal_slices-methods".

I am personally ok with either. IMHO, both are way better and safer than modifying index file on install.

[m7pr]

Hey, just pulling up what others wrote before:

@m7pr #335 (comment)

Separate documentation for utilities

@averissimo #335 (comment)

Having a small doc that directs to teal_slice(s) seems like the way to go.

@donyunardi #335 (comment)

Personally, I think we should just make the utility functions into a separate Rd file so that we can display only teal_slice and teal_slices in the index page.

@pawelru #335 (comment)

let's just simply break that relation, i.e. an idea to create a separate man page for those non-indexed objects.

If we create a separate page that is linked from ?teal_slices(s) it will be accessible for a user. We can have @keywords internal for that extra man page, so that it does not pollute index.

[m7pr]

Hey, I just looked at few other package indexes that we have, and they are also a bit messy.
I think having one page for teal_slice, another for teal_slices and separate pages for utility functions would still create a cleaner index than we have in {teal.data}, {teal.widgets} or {teal.transform}.

[m7pr]

@chlebowa your propositions of solutions undoubtedly showcases your high standards and unique approach, which we truly admire. It's evident that a lot of thought and expertise went into its development, and for that, we commend you. Your dedication to ensuring excellence and your willingness to go the extra mile haven't gone unnoticed by the team!

However, after looking at the previous discussions and considerations from other team members, I think I can tell we've collectively decided to move forward with a simpler solution: creating separate pages for utility functions (one for teal_slice-utilities and the other one for teal_slices-utilities). While we recognize the merits of your innovative approach, the consensus leans towards a solution that aligns more closely with standard practices (no editions to index.html during the installation of the package).

I want to assure you that your efforts haven't been in vain. In fact, your solution has provided valuable learning opportunities for everyone involved, enriching our understanding of the problem (package installation parts, creation of index file).

Before finalizing the decision, I wanted to check in with you to ensure you're comfortable with this direction. Your input is highly valued, and we want to make sure that we're all on the same page moving forward.

[chlebowa]

I still tea_slice and is.teal_slice not being on the same page is silly but do whatever you decide as long as teal_slice and teal_slices are on the index.

[m7pr]

Created a PR to solve this issue #572