search

miraisolutions / compareWith

RStudio Addins for Enhanced Diff and Merge
55 stars 11 forks source link

Review addins_factory evaluation #26

Closed riccardoporreca closed 5 years ago

riccardoporreca commented 5 years ago

See comment to commit https://github.com/miraisolutions/compareWith/commit/7b532033f7bb28241228d0b6348e8c4f8c62e967#r33984145

I might be missing something, and I think it would have been nice to pair this code change with an update to the existing unit tests revealing the cases the original implementation would not cover.

Overall, the idea is to use with_addin_errors() similar to withCallingHandlers(), which internally does not need to eval(expr).

I would also rather go for {...} (see the unit tests again) instead of expression(...) (and only when needed).

The commit was in fact a quick fix to an issue causing the addin binding not to re-execute after the first usage. This is due to the value body and being (lazily) evaluated instead of being kept as expression. The fix should be reviewed to preserve the intended meaning and usage of with_addin_errors() as described above.

riccardoporreca commented 5 years ago

The quick fix has been reverted and a failing unit test has been introduced in 752578edd731cc576925249c059540111e4e5946

riccardoporreca commented 5 years ago

Alternative implementations of a refactored addins_factory() implementation preserving expression: https://github.com/miraisolutions/compareWith/blob/71f570db8a7de06ababcd5d3bbb803d6b1306e8a/R/addins.R#L18-L40

All are working, TBD which one we prefer. Main aspects of the two approaches

  1. Simpler concept, however relying on lexical scoping, with all addin bindings having the same body.
  2. Based on more advanced implementation techniques, resulting in self-contained addin-specific body, same as if we would do it manually.

I have a general (but not strong) preference for (2) over (1) although it is a less trivial, more advanced implementation. Within (2), I like (2.1.1) more from a style perspective.

@RolandASc, @nfarabullini, Please go for the implementation you think is more suitable as part of the PR #29 review.