Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding calcUnique option to only pass unique values to anytime_cpp() … #110

Merged
merged 5 commits into from Jan 18, 2020

Conversation

@stephenbfroehlich
Copy link
Contributor

@stephenbfroehlich stephenbfroehlich commented Jan 18, 2020

…and thereby speed up calcualtions with repeated inputs to x.

…and thereby speed up calcualtions with repeated inputs to x.
@stephenbfroehlich stephenbfroehlich marked this pull request as ready for review Jan 18, 2020
@stephenbfroehlich
Copy link
Contributor Author

@stephenbfroehlich stephenbfroehlich commented Jan 18, 2020

Sorry about all of the whitespace changes ... I can't figure out how to turn them off in the pull request.

@codecov
Copy link

@codecov codecov bot commented Jan 18, 2020

Codecov Report

Merging #110 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #110   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           5      5           
  Lines         230    237    +7     
=====================================
+ Hits          230    237    +7
Impacted Files Coverage Δ
R/anytime.R 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c7c2c28...25dbff3. Read the comment docs.

@eddelbuettel
Copy link
Owner

@eddelbuettel eddelbuettel commented Jan 18, 2020

Two things, quickly:

  1. Yes, the complete whitespace change is a mess. But we can fix. Are you by chance on Windows? It may then matter how you configure your git client. There are smarter ways to go about "autoMAGICally" fixing/adjusting. Methinks you force-changed, likely inadvertently. Or you use a truly terribly editor from way before even the dark ages.

  2. Here at GitHub in the 'Files Changed' view, you can click on the (small) 'Settings' icon in the middle and you should see this:

image

So select 'hide all whitespace changes' and then hit 'reload'.

  1. Your diff is very clean and looks good. Apart from that issue in 1) we're likely good.

  2. These days, the repo owner (me) can actually push back changes so lemme try to 'clean' your file.

@eddelbuettel
Copy link
Owner

@eddelbuettel eddelbuettel commented Jan 18, 2020

As I suspected:

edd@rob:~/git/anytime/R(pr-110)$ file anytime.R     # on your branch, locally checked out
anytime.R: ASCII text, with CRLF line terminators
edd@rob:~/git/anytime/R(pr-110)$ git co -
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
edd@rob:~/git/anytime/R(master)$ file anytime.R   # master
anytime.R: ASCII text
edd@rob:~/git/anytime/R(master)$ 
@eddelbuettel
Copy link
Owner

@eddelbuettel eddelbuettel commented Jan 18, 2020

@stephenbfroehlich I presume you are ok with the changes I made

  • converted back from CRLF as discussed
  • added simple unit test
  • added some more #nocov tags to get the coverage back
@stephenbfroehlich
Copy link
Contributor Author

@stephenbfroehlich stephenbfroehlich commented Jan 18, 2020

Yes, you're correct.
What I did was edit it originally in Windows (in RStudio, but git did check it out with CRLFs) ... and then move the file over to Linux for testing and the actual merge ... which is how git missed that on the return journey. Thanks for getting it fixed.

The extra testing lines were something I simply wasn't familiar with to the point where I thought to anticipate them, and code testing coverage is a completely new concept to me, so I'm just following along to learn at this point. So thanks ... and it all looks good ...

As far as I can tell, there is nothing for me to do to formally approve the new commits ... is that right?

@eddelbuettel
Copy link
Owner

@eddelbuettel eddelbuettel commented Jan 18, 2020

Code coverage is a bit of a "game" - I was initially against it because it too obviously cheaply overriden as I have done here, but it has also saved me in the past. Tests are good, so focus more on the (simple, good enough) one I added: simply comparing results with / without your change.

Thanks again for the PR. We are good now -- I was the one approving your changes (and then we inverted it informally as I made some changes). Glad you got it all working. One, last, final tip: I even move between machines via git(hub): commit from (Windows) laptop, onto branch or fork where no "harm is done", check out on Linux box, test some more, push back to git(hub) if needed. One of the key things with git that will really grow on you is ... that all that messy manual scp or dropbox or ... business is gone and ALL changes become transactional, and thereby reversible. It's worth it.

@eddelbuettel
Copy link
Owner

@eddelbuettel eddelbuettel commented Jan 18, 2020

And I just made one more change I had meant to make earlier: immportalise you in ChangeLog.

@stephenbfroehlich
Copy link
Contributor Author

@stephenbfroehlich stephenbfroehlich commented Jan 18, 2020

With my own repos, I do that ... I just had no idea how to check out yours until today.

Thanks again.

@eddelbuettel eddelbuettel merged commit cc838eb into eddelbuettel:master Jan 18, 2020
3 checks passed
3 checks passed
codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 100% (+0%) compared to c7c2c28
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.