Pr/1183

[traversc]

I added some syntax checking based on @Enchufa2 's feedback.

Tests here: https://github.com/traversc/RcppTest/tree/master/inst. These tests incorporate the examples from @Enchufa2.

Currently, you can run them with make install && make test, but I'm not sure the best way to add the tests as a pull request.

[eddelbuettel]

Currently, you can run them with make install && make test, but I'm not sure the best way to add the tests as a pull request.

Hm, can you explain how that is supposed to work when a) we have no Makefile and b) nobody calls make test and c) you only committed to src/attributes.cpp?

It has been a while since I wrote new non-standard tests but we have a lot of them in the package you could study. There are even ones for package building etc (though not all may run at all times). One could attempt building a package (including a call to compileAttributes() invoking the attributes parser on expression meant to break. Not quite sure right now how to test multiple breaking expressions though without having to concoct multiple packages. Maybe by copying variants of a source file in, attempting a build for each and logging the expected failure.

[Enchufa2]

If @traversc is not familiar with tinytest or unit testing in general, I can help with that.

[eddelbuettel]

Me too, of course. Always happy to help with tinytest which, via its "it is just a script" approach really makes it easy.

[Enchufa2]

@traversc Checking your RcppTest repo, I see that you made a package specifically to test this. So the idea would be to integrate those tests into inst/tinytest as part of this PR, removing the manual prints, stopifnots, etc., and replacing them with the proper tinytest utilities that Rcpp already uses. You can take a look there to see how other similar tests are implemented.

[Enchufa2]

Also, @traversc you can check past PRs with added tests. For example: #1180 (very simple one) and #1139 (more complex tests).

[eddelbuettel]

And we have done just that: embed mock packages bnelow inst/tinytest/ to be used (only !!) by the tests.

[traversc]

Okay, I added some tiny tests. The test script is test_attribute_package.R which builds the package testRcppAttributePackage.

The test script tests two things:

1. that the package builds and the signature attribute produce the expected results
2. that it also works inline (sourceCpp) and gives you an error when incorrect syntax is used (rather than falling through and segfaulting ;))

The test package only contains correct syntax so doesn't test for handling incorrect syntax within a package, but since it's the same code path to build R function signatures inline and via package, I think that's OK?

[Enchufa2]

Comments inline. Also, please:

• Add yourself to the copyright header in the src/attributes.cpp file.
• Rebase this branch to avoid conflicts with the latest changes in Adding Rcpp signature attribute #1183.

[Enchufa2]

Did you test this? Does this fail on Windows?

[traversc]

I went ahead and tested it on windows.

[Enchufa2]

And... does it work?

[traversc]

Tested and working on windows (and also in CI).

[traversc]

@Enchufa2 Replies inline. A bunch of commits for minor things, but hopefully they can be squashed at the end.

Also, one comment: I noticed the ChangeLog is a mixture of spaces and tabs -- is that intentional?

[Enchufa2]

Looking very good, thanks for all the changes! I left a few more comments. I also activated the CI and everything passes (don't worry about the covr report).

@Enchufa2 Replies inline. A bunch of commits for minor things, but hopefully they can be squashed at the end.

Sure, no problem.

Also, one comment: I noticed the ChangeLog is a mixture of spaces and tabs -- is that intentional?

Yeap, I asked the same thing some time ago, but I don't recall @eddelbuettel's answer. :)

[eddelbuettel]

Spaces vs tabs is accidental, we don't have control over all the editors in the world. I usually just resists global changes across the file as large white-space only commits are just noise. I can clean it up another time (there is yet another emacs function for it that does it well).

[eddelbuettel]

(don't worry about the covr report).

Yes. We actually addressed that in the branch this would merge into.

So thanks for making all the changes (even if you some commits are sequentlial with the same commit message --> I prefer the amend option and force-pushing which is admissable to branches). (Just bringing this up as we're at the topic of gentle nags now ;-) )

[traversc]

Updated, hopefully I addressed everything properly this time :)

[eddelbuettel]

That looks pretty good now. There are two trivial whitespace things I might change but it's not worth making an issue over now.

[Enchufa2]

LGTM, thanks! We can squash and merge this into #1183#1184, and then ping JJ to take a last look at src/attributes.cpp there?

[eddelbuettel]

Yep that's the idea: by squashing it into our pr/1183 branch the pending PR #1184 should auto-update.

Giving that a whirl now.