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

Closes #9, mistaken test condition #10

Merged
merged 2 commits into from Oct 24, 2018

Conversation

@MichaelChirico
Copy link
Contributor

@MichaelChirico MichaelChirico commented Oct 23, 2018

is.character('unit') is trivially TRUE for all values of unit argument, i.e. this test was on the variable name, not the variable itself.

Very simple fix; more importantly, test added to be sure this type test is working as intended.

Bug fixed is #9.

The second commit tinkers with .gitignore.

  • .Rproj files are very small but don't really add much (e.g.)
  • Having the trailing slash / on the .RProj.user folder was confusing RStudio for some reason. It kept trying to edit the .gitignore to add .RProj.user to the end... really an upstream bug but it doesn't hurt to make this fix.
@joshuaulrich
Copy link
Owner

@joshuaulrich joshuaulrich commented Oct 23, 2018

The test looks great. The only changes I would ask for are cosmetic/preferences.

  1. Undo the whitespace changes to the doxygen comments in lines 51-54.
  2. Remove the addition of *.Rproj to .gitignore from the fix commit (put it in a separate commit if you think it's a useful change).
  3. Amend the commit message according to contributing guidelines (link is to xts, because I haven't added one to this repo; apologies).

I can make any/all of these changes, if you prefer. I only need access to push to this branch.

@MichaelChirico
Copy link
Contributor Author

@MichaelChirico MichaelChirico commented Oct 23, 2018

@joshuaulrich working now...

put it in a separate commit if you think it's a useful change

A separate commit is OK, or you a separate PR?

@joshuaulrich
Copy link
Owner

@joshuaulrich joshuaulrich commented Oct 23, 2018

@MichaelChirico
Copy link
Contributor Author

@MichaelChirico MichaelChirico commented Oct 23, 2018

@joshuaulrich should be ready to merge, I think. In any case I added you as a collaborator to my fork in case you want to tinker.

@joshuaulrich joshuaulrich merged commit aa2c1a1 into joshuaulrich:master Oct 24, 2018
1 check passed
@joshuaulrich
Copy link
Owner

@joshuaulrich joshuaulrich commented Oct 24, 2018

Merged, thanks!

@MichaelChirico
Copy link
Contributor Author

@MichaelChirico MichaelChirico commented Oct 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants