Replacing testthat with tinytest (Issue #136) #137
Conversation
…n CRAN either. So match to some of the message instead of all.
…ts: expect_true(grepl…
2) Remove mention of testtthat runner from init_nlopt.c and nlopt.h 3) Add dummy.cpp to trigger g++ linking for libnlopt 4) Remove testthat from DESCRIPTION
2) Clean up Description a bit now that we are in R 4.3+
…to trigger tests. In general, most of these errors are caught in nloptr.R and never get to is.nloptr.
… not Sourceforge :)
… as well (thank goodness for version control), correct missing comment indicator, and rebuild documentation.
There was a problem hiding this comment.
This is a bad PR in the sense that those of use working professionally with GitHub prefer a seperation of concerns between unrelated issues.
You did a momentous job here switching to tinytest. It gets completely blurred by all these nonsense whitespace changes from lintr (a package I am not on friendly terms with because I find it way too invasive, and tend to disagree with a number of its default rules more or less on principle [default line length? seriously? like in 1992 when we were on 80x25 screens?]. Anyway...
(The key is the tests pass, and if @astamm is cool with it all I am certainly not objecting. But I don't have the time right to dive into a 93 file PR that is very distracting because a million changes are in fact just whitespace ...)
|
Hi, @eddelbuettel. No arguments, per se. I have bad habits from being predominantly a solo developer for the past 25 years, and need to work harder on creating better submissions for others, so I appreciate the constructive criticism. So as to assuage my ego just a tad, there was some method to my madness 😁 . When it came to changing the the code I did not write, namely the But yes, that clearly should have been a second PR, and the vignette update a third. Thank you! |
|
Yep. Small(er) iterations tends to be better. But I also sit on more feedback doing these kinds of things all day long. Again, first and and foremost thank you for carrying the conversion through. I am a happy (and early) adopter of |
|
I say we keep in mind to separate things in more smaller PRs from now on. If it is ok with everyone, I'll just merge this one as it is. Very nice work @aadler and again thank you! |
The
testthatframework was repalced bytinytestand a few extra tests were written to ensure we stay at > 95% coverage. Code and documentation were linted to become more consistent. Not completely, because I am not as comfortable with roxygen (I write pure .Rd for my packages) and I tried to err on the side of caution. Vignette was gently updated (e.g. Github, not Sourceforge 😄 ). It passes all unit tests and checks on my end (except pkgedown), so I think it's ready for merging.