linelist full package review

[Bisaloo]

Not to be merged. See https://epiverse-trace.github.io/blueprints/code-review.html#full-package-review.

[chartgerink]

Overall a really solid package. Happy to approve this!

I only left comments and questions for your consideration, and for future discussion.

1. The package supports data.frame and tibble. As I understand it, data.table is also getting quite popular. Any thoughts on whether we want to include this in the future?
2. The function naming can be slightly confusing at times.

• For example, tags_names and tags_defaults for some reason feel odd as they have a double plural. Here I keep typing tag_names and tag_defaults myself.
• Another example is lost_tags_action which is past tense and might be more literal by being active and present - e.g., lose_tags_action or losing_tags_action

3. Do we want to have the discussion around object or column tagging here or park it for later, when the code is abstracted into a separate package? I don't know how important or needed the tag viewer is, proportional to the work that is needed and am raising it here because it seems relevant to the full package review domain.
4. There are a few for loops in the code which might be candidates for rewrite using apply or a variant of it. I find for loops more readable myself, but I've come to learn that it's more common to write these functions instead.

I am still catching up on some of the ways the operators are defined for classes in R (e.g., [[<-.linelist <- function`) so those were a bit harder to review at this time.

[Bisaloo]

Overall a really solid package. Happy to approve this!

Thanks for the comments!

1. The package supports data.frame and tibble. As I understand it, data.table is also getting quite popular. Any thoughts on whether we want to include this in the future?

Probably not. Its interface differs too much from standard data.frames and it would be extremely challenging to make it fit in our framework and cover all edge cases.

2. The function naming can be slightly confusing at times.

No super strong opinions on this and renaming some of these functions could be done while extracting & generalizing the tagging functionality

3. Do we want to have the discussion around object or column tagging here or park it for later, when the code is abstracted into a separate package? I don't know how important or needed the tag viewer is, proportional to the work that is needed and am raising it here because it seems relevant to the full package review domain.

There are some useful elements in epiverse-trace/datatagr#32 but agreed this is a discussion for 2.0.0

4. There are a few for loops in the code which might be candidates for rewrite using apply or a variant of it. I find for loops more readable myself, but I've come to learn that it's more common to write these functions instead.

This blog post will hopefully convince you to give apply and its variant more love: https://epiverse-trace.github.io/posts/for-vs-apply/. 😉

This issue is now tracked in #105.