Add an argufy_package function #5

jimhester · 2015-08-11T13:52:26Z

This function will call argufy() on all of the functions within a
package. Current implementation should work for all functions including
S4 generics and methods.

I added a simple test case, I should probably flesh out the documentation more as well. I wanted to get some feedback before going further.

Let me know if the implementation is unclear, I happy to change things if needed!

This function will call argufy() on all of the functions within a package. Current implementation should work for all functions including S4 generics and methods.

gaborcsardi · 2015-08-11T14:33:29Z

tests/testthat/test-argufy_package.R

+  old <- .libPaths()
+  .libPaths(temp_lib)
+  on.exit(.libPaths(old))
+  tools:::.install_packages("TestS4")


I don't think this will go through CRAN. Btw. why is it better than install.packages?

CRAN doesn't check code in tests, but I agree it is not better than install.packages, I forgot you could pass file paths to install.packages.

gaborcsardi · 2015-08-11T14:39:14Z

Hmmm. I am not really sure about this, to be honest. My problem is that the functions with the assertions are broken, unless you run argufy() on them. So why not just put the argufy() calls in the code? If you need to modify them anyway?

I was actually first considering another approach, where the assertions are in comments, and then the functions are never broken. E.g.

f <- function(
    x = 5,         #: is.numeric
    y = "foo",     #: is.character
) {
   ...
}

But then decided that code should be in code, not in comments....

gaborcsardi · 2015-08-11T14:48:35Z

tests/testthat/TestS4/DESCRIPTION

+Version: 0.0.0.9000
+Authors@R: person("Jim", "Hester", email = "james.f.hester@gmail.com", role = c("aut", "cre"))
+Description: What the package does (one paragraph).
+Depends: R (>= 3.3.0)


Do we really need R 3.3.0 for this? That is kinda bad.

No, devtools::create() just puts the current R version in there by default, definitely can use prior versions.

gaborcsardi · 2015-08-11T14:49:46Z

Let me think about it a bit. Or can you convince me that this is a good idea? :)

jimhester · 2015-08-11T15:30:17Z

My main motivation is simply convenience, rather than having to wrap every function call you use with argufy() you can just add the argufy annotations throughout and a call to argufy_package() at the end.

This also makes it easier IMHO to argufy an existing package incrementally, just by adding the annotations to the function arguments you want to check (and the argufy_package() call).

While it is true the functions are broken until argufy'd I generally develop using calls to devtools::load_all(), in which case they will always be correct. I agree if you are copy pasting the function definitions this method has a drawback.

I guess the nice thing is if you want to use argufy() throughout you can do so interchangeably with argufy_package(), as argufy() is idempontent.

f1 <- function(x = ~ is.character(x)) { x }
all.equal(argufy(f1), argufy(argufy(f1)))

So you can be explicit where you would like, and lazy elsewhere.

gaborcsardi · 2015-08-11T15:53:21Z

I don't have strong objections. It is really a pity that (seemingly) there is no syntax we could use, that is just ignored by R (except for comments, of course). I mean, there are some options, but all I could come up with was rather clumsy.

gaborcsardi · 2015-08-11T15:58:12Z

How about this:

f <- function(
  x = assert_integer(10)),
  y = as_character("foo"),
  z
) {
  x
}

and then we define assert_integer and as_character, etc. to be identity.

It is worse than an operator, though, imho, less readable.

EDIT: or we could define our own iterator, like %where%, but the % is clumsy, too:

f <- function(
  x = 10       %where% is.integer,
  y = "foo"    %via% as.character,
  z
) {
  x
}

jimhester · 2015-08-11T16:01:26Z

I guess you could use magrittr pipes for this then

f <- function(
  x = 10 %>% assert_integer,
  y = 100 %>% as_character,
  z
) {
  x * y
}

gaborcsardi · 2015-08-11T16:03:34Z

Yeah, see my edits. (Probably using the pipes is overkill, though, they do too much, we just need a simple operator or two.)

This is actually not too bad. Now all we need is to find some nice name(s) instead of the where and via in the examples.

EDIT: The operator is also nice, because then we can just use ordinary functions like is.integer is we want, no need to write separate assert_integer and co.

jimhester · 2015-08-11T16:09:19Z

We could actually use := directly

`:=` <- function(x, y) x

f <- function(
  x = 10       := is.integer,
  y = "foo"    := as.character,
  z
) {
  x
}

gaborcsardi · 2015-08-11T16:11:34Z

Yeah, I thought about that. My problem is that if you have no default values, then you need a new syntax. The tilde is a nice operator, because it is both binary and unary. How would you specify arguments with no default values with :=?

gaborcsardi · 2015-08-11T16:14:34Z

My idea was something like

. <- quote(expr = )
`:=` <- function(x, y) x

f <- function(
  x = .       := is.integer,
  y = "foo"   := as_character,
  z 
) {
  x
}

but then this error message is a little hard to interpret:

> f()
Error in `:=`(., is.integer) (from assert.R!16681L2F#1) : object '.' not found

gaborcsardi · 2015-08-11T16:17:39Z

Also, data.table exports :=. (It really should not export it, though.) So maybe a % operator is better, after all. The no default value problem applies to that, too, of course.

jimhester · 2015-08-11T16:24:50Z

This seems to work

`.` = "__placeholder__"
`:=` <- function(x, y) if (x == "__placeholder__") quote(expr=) else x
f <- function(
  x = .       := is.integer,
  y = "foo"   := as_character,
  z 
) {
  x
}

%==% or %=% or %:= are all feasible alternatives I guess, it would be nice to use := but the data.table issue probably precludes it.

gaborcsardi · 2015-08-11T16:28:30Z

How about ? and/or !?

`?` <- function(x, y) x

f <- function(
  x =         ?! is.integer,
  y = "foo"   ?! as_character,
  z 
) {
  y
}

f()

This is both unary and binary, and probably not used in packages at all, so it would be OK to import our ?? Also, if the RHS is not a !, we can just revert to utils::?.

gaborcsardi · 2015-08-11T16:31:48Z

I think this is getting good. I like the ?! more than %==%, etc., it is less clumsy imo.

We can go with or without the placeholder, maybe without is better. If with, then it probably needs to be something else than a dot. Packages using magrittr often need to define the dot as a global variable, to pass CRAN checks.

gaborcsardi · 2015-08-11T16:34:35Z

We can also have ?? for assertions and ?! for conversions. It is also possible to have ?~ for something. Maybe this is getting a bit too much. :)

jimhester · 2015-08-11T16:38:02Z

I like the ? idea and I agree without the placeholder is best if possible. I guess we could do ?+ as well, though I am not sure what that would be for, but it is good to have flexibility.

gaborcsardi · 2015-08-11T16:42:09Z

Yeah, we can have any combinations of unary operators, possibly starting with ? so that it is not called all the time if the function is not argufy()d. I do like ?+ as well, and ?-, too.

I guess the question is which one is the most readable. It would be nice to have a way to read out the function definition, including the assertions.

gaborcsardi · 2015-08-11T16:43:49Z

Btw. do you think the argufy name is appropriate? It sounds funny and cool to me, but I have no idea what associations a native speaker might have..... :)

jimhester · 2015-08-11T17:00:07Z

argufy seems ok to me. I keep forgetting it is not argify, there are a large number of English words with the -ify suffix (https://en.wiktionary.org/wiki/Category:English_words_suffixed_with_-ify). But as these are arguments I don't think having the u is horrible, but it probably makes it a little more awkward to pronounce out loud.

gaborcsardi · 2015-08-11T17:08:09Z

Thanks. Both argify and argufy are wrong in the sense that we are not creating arguments here. I chose argufy because it is a word. :) Anyway, we can change it later to something that has to do with assertions, maybe assertify or certify is better.

Btw. There is a discussion with Stefan about merging ensurer and argufy: smbache/ensurer#9 (comment)
FYI.

jimhester · 2015-08-11T17:35:44Z

That is true (although I have never heard anyone use argufy as a word before 😄)

Re: merging with ensurer, I leave that decision to you, although I kind of like having this package focused solely on build time checking, the ensurer functions serve a slightly different (but related) purpose.

gaborcsardi · 2015-08-11T18:04:40Z

ensurer::function_ can be a build-time dependency, too, as long as you don't actually use any checks from ensurer. So, yeah, not in its current form, but I would definitely want to keep it as a build time dependency.

gaborcsardi · 2015-08-11T18:31:26Z

I'll rewrite the package with ?! as assertions and ?~ as coercions.

?! would read as 'error if not'. E.g. x = 10 ?! is.integer reads as "x defaults to 10, error if not is integer".

?~ would read as 'error if can't use'. E.g. x = 10 ?~ as.integer reads as "x defaults to 10, error if can't use as integer".

I'll merge this PR, once ready. If you are suffering with disposables let me know. Sometimes it works with devtools::test() locally, but does not during R CMD check.

jimhester · 2015-08-11T19:03:46Z

Not really sure why the tests are failing on the CI's, they pass both with devtools::test() and R CMD check on my machine. It looks like installing the TestS4 package is failing to install, maybe disposables is not adding methods to the dependencies, or I am using it incorrectly somehow?

gaborcsardi · 2015-08-11T19:33:25Z

R/package.R

+  # these statements are needed to get S4 functions to work properly
+  was_s4 <- isS4(fun)
+  old_attributes <- attributes(fun)
+


Btw. I think it is nice to have independent changes like this and the next chunk in a separate commit.

Also, this is not quite good, because attr(, "srcref") is copied as well, and it is used for printing the (new) function. I am not entirely sure what to do with this, though. If we don't copy, then the new function will have no srcref, which might be a problem sometimes, no? But we also do not want to reparse the function....

That is true. In some respects the srcref version is more useful interactively, because the function definition is not cluttered by the additional checking code (which will become a larger problem when inlining functions (#6)). On the other hand the printed function is not exactly what is called by R, which may make things confusing.

It is possible to view the real source with print(fun, useSource = FALSE) even with the copied srcref.

The only downside to not having a srcref at all is that you lose comments and formatting, because R then simply prints the parsed expressions, it shouldn't actually cause any errors anywhere though.

I personally would lean towards leaving the srcref copied as it is, but if you feel it is confusing simply dropping that attribute shouldn't cause any problems.

If the code can be printed easily then it is fine as it is now. I'll just add it to the docs. This confused me until I debugged argufy(), because it did not seem to do anything. :)

Agreed I was confused as well, easy way to see the real function definition
is body(fun) or print(fun, useSource = FALSE)

On Tue, Aug 11, 2015 at 4:41 PM, Gábor Csárdi notifications@github.com
wrote:

In R/package.R
#5 (comment):

@@ -50,6 +50,10 @@
argufy <- function(fun, ...) {
if (!is.function(fun)) stop("'fun' must be a function")

these statements are needed to get S4 functions to work properly

was_s4 <- isS4(fun)

old_attributes <- attributes(fun)

If the code can be printed easily then it is fine as it is now. I'll just
add it to the docs. This confused me until I debugged argufy(), because
it did not seem to do anything. :)

—
Reply to this email directly or view it on GitHub
https://github.com/gaborcsardi/argufy/pull/5/files#r36795931.

gaborcsardi · 2015-08-11T19:36:11Z

OK, so apart from the CI failures, is this ready to be merged? Or you want to do sg else?

jimhester · 2015-08-11T19:54:04Z

I modified the tests per your suggestions, so this can be merged, unless we want to figure out the CI issues first.

gaborcsardi · 2015-08-11T19:55:46Z

We can figure them out later, maybe we just need Sys.setenv("R_TESTS"="")

Add an argufy_package function

gaborcsardi · 2015-08-11T21:00:21Z

?! is actually not good, because you it is hard to parse things like ?! is.integer(x) && length(xx) == 1. Parentheses would be required. So maybe it'll be ? and ?~ for coercions.

jimhester · 2015-08-11T21:03:57Z

I think ? and ?~ are fine, just as readable really and one fewer
character!

On Tue, Aug 11, 2015 at 5:00 PM, Gábor Csárdi notifications@github.com
wrote:

?! is actually not good, because you it is hard to parse things like ?!
is.integer(x) && length(xx) == 1. Parentheses would be required. So maybe
it'll be ? and ?~ for coercions.

—
Reply to this email directly or view it on GitHub
#5 (comment).

gaborcsardi · 2015-08-11T21:28:13Z

OK, basically done, I'll write the tests in the evening and then merge.
https://github.com/gaborcsardi/argufy/tree/qmark-notation

Add an argufy_package function

3eaeede

This function will call argufy() on all of the functions within a package. Current implementation should work for all functions including S4 generics and methods.

gaborcsardi reviewed Aug 11, 2015
View reviewed changes

Use disposables

ecf7069

Do not qualify packages, they are already loaded

abee667

Remove test package

8874c44

gaborcsardi reviewed Aug 11, 2015
View reviewed changes

Test changes suggested by Gabor

ff9c05f

gaborcsardi added a commit that referenced this pull request Aug 11, 2015

Merge pull request #5 from jimhester/argufy_package

e650291

Add an argufy_package function

gaborcsardi merged commit e650291 into gaborcsardi:master Aug 11, 2015

Add an argufy_package function #5

Add an argufy_package function #5

Conversation

jimhester commented Aug 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaborcsardi commented Aug 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaborcsardi commented Aug 11, 2015

jimhester commented Aug 11, 2015

gaborcsardi commented Aug 11, 2015

gaborcsardi commented Aug 11, 2015

jimhester commented Aug 11, 2015

gaborcsardi commented Aug 11, 2015

jimhester commented Aug 11, 2015

gaborcsardi commented Aug 11, 2015

gaborcsardi commented Aug 11, 2015

gaborcsardi commented Aug 11, 2015

jimhester commented Aug 11, 2015

gaborcsardi commented Aug 11, 2015

gaborcsardi commented Aug 11, 2015

gaborcsardi commented Aug 11, 2015

jimhester commented Aug 11, 2015

gaborcsardi commented Aug 11, 2015

gaborcsardi commented Aug 11, 2015

jimhester commented Aug 11, 2015

gaborcsardi commented Aug 11, 2015

jimhester commented Aug 11, 2015

gaborcsardi commented Aug 11, 2015

gaborcsardi commented Aug 11, 2015

jimhester commented Aug 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

these statements are needed to get S4 functions to work properly

gaborcsardi commented Aug 11, 2015

jimhester commented Aug 11, 2015

gaborcsardi commented Aug 11, 2015

gaborcsardi commented Aug 11, 2015

jimhester commented Aug 11, 2015

gaborcsardi commented Aug 11, 2015