search

epiverse-trace / linelist

R package for handling linelist data
https://epiverse-trace.github.io/linelist/
Other
8 stars 4 forks source link

linelist full package review #103

Closed Bisaloo closed 7 months ago

Bisaloo commented 7 months ago

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

Bisaloo commented 7 months ago

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.

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

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

  1. 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/. :wink:

This issue is now tracked in epiverse-trace/linelist#105.