search

tdaverse / ggtda

ggplot2 extension to visualize persistent homology
https://tdaverse.github.io/ggtda/
GNU General Public License v3.0
21 stars 6 forks source link

import rlang + query simplextree_version on load + calibrate engine to version #45

Closed corybrunson closed 2 years ago

corybrunson commented 2 years ago

This commit does three things:

  1. Use control flow to use appropriate syntax in the {simplextree} engine if version 0.9.1 is installed.
  2. Create a hidden object .simplextree_version to assist (1).
  3. Import {rlang} to enable (2).

I've tested the examples with both versions of {simplextree} installed.

It would be feasible to do (1) differently, by querying the version of {simplextree} each time it's needed. It's not clear to me that the convenience is worth the additional import; i did it this way in part to experiment with {rlang}'s on_load() and run_on_load() functions.

corybrunson commented 2 years ago

Whoops—the commit also

  1. Un-imports some specific functions implemented in {simplextree} later than v0.9.1, which aren't necessary because they're explicitly accessed via :: in the code (and aren't used unless v1.0.1 or greater is installed) but cause warnings when v0.9.1 is installed.

I think imports can be dropped regardless of which approach we take, so i'll remove them in an edit.

corybrunson commented 2 years ago

@jamesotto852 any thoughts on this PR? I think we should remain compatible with v0.9.1 until {simplextree} and {Mapper} are reconciled, but i'm open to dropping the hidden object solution since the version query is only performed twice.

jamesotto852 commented 2 years ago

I think this looks good! I am definitely open to importing {rlang}, it's already an import in {ggplot2} so I don't think any {gghdr} users will need to install it. I would also be happy to leverage its error handling tools in future updates, if that's something others would be interested in.

corybrunson commented 2 years ago

Oh, good point—we're not really introducing a dependency then. And yes, i am in favor of taking full advantage of it.

Since the PR is directed at your branch, i'll leave it to you to merge. : )