search

astamm / nloptr

nloptr provides an R interface to NLopt, a free/open-source library for nonlinear optimization providing a common interface to a number of different optimization routines which can handle nonlinear constraints and lower and upper bounds for the controls.
https://astamm.github.io/nloptr/
Other
103 stars 36 forks source link

Replacing testthat with tinytest (Issue #136) #137

Closed aadler closed 10 months ago

aadler commented 10 months ago

The testthat framework was repalced by tinytest and 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.

aadler commented 10 months ago

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 .R files written mainly by @jyypma and some by @hwborchers, I was afraid of making a breaking change, so I intentionally placed each one in its own commit in case I needed to backtrack. The files I wrote I committed in one fell swoop. Of course, that was the one in which I made an error, but that's just the universe keeping me in my place 😄 .

But yes, that clearly should have been a second PR, and the vignette update a third. Thank you!

eddelbuettel commented 10 months ago

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 tinytest and like it a lot.

astamm commented 10 months ago

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!