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

fixed a bug where the kitten function errored out on tinytest, when c… #18

Merged
merged 3 commits into from
Nov 15, 2023
Merged

Conversation

paulhodor
Copy link
Contributor

…alled with a non-default path; fixed a typo in the help page of kitten

…alled with a non-default path; fixed a typo in the help page of kitten
message(" >> added tinytest support")
tinytest::setup_tinytest(name, verbose=FALSE)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I messed up root vs name here? That's certainly possibe.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The things is ... that it works as is. (Expand for log)

edd@rob:/tmp$ mkdir kitten
edd@rob:/tmp$ cd kitten/
edd@rob:/tmp/kitten$ kitten.r -h\
> ^C
edd@rob:/tmp/kitten$ kitten.r -h
Usage: kitten.r [-t TYPE] [-b] [-p] [-h] [-x] PACKAGE

-t --type TYPE      type of kitten: plain, rcpp, arma, eigen. [default: plain]
-b --bunny          install roxygen2 documentation example and roxygenize (only for plain)
-p --puppy          invoke tinytest::puppy to set up testing (only for plain)
-h --help           show this help text
-x --usage          show help and short example usage 
edd@rob:/tmp/kitten$ kitten.r -p quickcheck
Creating directories ...
Creating DESCRIPTION ...
Creating NAMESPACE ...
Creating Read-and-delete-me ...
Saving functions and data ...
Making help files ...
Done.
Further steps are described in './quickcheck/Read-and-delete-me'.

Adding pkgKitten overrides.
 >> added .gitignore file
 >> added .Rbuildignore file
 >> added tinytest support
Deleted 'Read-and-delete-me'.
Done.

Consider reading the documentation for all the packaging details.
A good start is the 'Writing R Extensions' manual.

And run 'R CMD check'. Run it frequently. And think of those kittens.


Thank you edd, for showing us some PUPPY LOVE <3

    ,-.___,-.
    \_/_ _\_/
      )O_O(
     { (_) }  W00F!
      `-^-'   
edd@rob:/tmp/kitten$ ls -l quickcheck/tests/tinytest.R 
-rw-rw-r-- 1 edd edd 94 Nov 15 12:49 quickcheck/tests/tinytest.R
edd@rob:/tmp/kitten$ ls -l quickcheck/inst/tinytest/test_quickcheck.R 
-rw-rw-r-- 1 edd edd 56 Nov 15 12:49 quickcheck/inst/tinytest/test_quickcheck.R
edd@rob:/tmp/kitten$ 

How did you call it to trigger an error? Maybe I get lucky here via pkgKitten.r from litter ?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I think you just answered in #19.

@eddelbuettel
Copy link
Owner

Could I ask you to add yourself to the ChangeLog file ? Format is pretty standard: name, two spaces, date, two spaces, email and then eight spaces and a * followed by the filename and one or two line change summary.

2023-11-15 Paul Hodor <paul@aurynia.com>

* R/pkgKitten.R: fixed path in call to tinytest; fixed typo in doc
of kitten()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

ChangeLog Outdated Show resolved Hide resolved
Copy link
Owner

@eddelbuettel eddelbuettel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for bug fix!

@eddelbuettel

This comment was marked as outdated.

@paulhodor
Copy link
Contributor Author

This is way too much automation for me. I did not realize that changes to my fork get propagated to you. I had made a mistake in the changelog and was working on fixing it. I wasn't aware that you can see all that activity.

@eddelbuettel
Copy link
Owner

Fair point but I think it makes sense in the context of a pull request which links a partitcular branch of your fork to my repo. Being able to dynamically update (wilder still: I get to write to it too !!) is generally good for PRs. But I understand your surprise.

No harm done though and force-push is good for this. I usually merge as is (whereas I squash in other repos). Ok with you?

@paulhodor
Copy link
Contributor Author

I added tests for the hello function, per #20. Looks like it went into the same pull request. I guess I should have created a new branch.

@eddelbuettel
Copy link
Owner

Looks like it went into the same pull request. I guess I should have created a new branch.

(Just playing "git doctor" here: correct. If you want that, you could do the following:

  • create a new branch of your current branch, that is basically for safekeeping
  • roll this branch back via eg git reset --hard HEAD^ should do, I usually reset to a sha1 as I am paranoid)
  • force push the rolled back branch to restore this PR
  • wait til I merge, pull, then create a new branch
  • cherry pick the third commit here and on the safekeep branch into the new branch, PR)

If it is too much work we just merge this. Your call. It's kinda nice to have two separate PRs but it doesn't matter greatly.

@paulhodor
Copy link
Contributor Author

Let's leave it as is. But thanks for the explanation. I'll remember it for next time.

* R/pkgKitten.R: replace auto-generated test_<name>.R with content
of test_hello.R

2023-11-15 Paul Hodor <paul@aurynia.com>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll clean this up post merge. It's generally an entry per day, not per commit. So these two will get fused.

@eddelbuettel eddelbuettel merged commit 0bab87a into eddelbuettel:master Nov 15, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants