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

first version of a check_ function #104

Merged
merged 21 commits into from Dec 17, 2017
Merged

Conversation

@EdwinTh
Copy link
Contributor

@EdwinTh EdwinTh commented Dec 8, 2017

added

  • check_noNA
  • unit tests

adjusted

  • step function
@topepo
Copy link
Collaborator

@topepo topepo commented Dec 9, 2017

I just checked in a bunch of changes to your PR. I made some modifications (I proposed check_missing instead of check_noNA. There were very few changes to the core of recipes. It passes all of the checks and I updated the documentation.

Can you sync and re-submit?

@EdwinTh
Copy link
Contributor Author

@EdwinTh EdwinTh commented Dec 15, 2017

I am a bit lost in the woods with all the repos now. I can't find where exactly you made these changes. The "Files changed" and the "Commits" only show my additions. I can't find anything in the fork either. Can you point me to where to look?

@topepo
Copy link
Collaborator

@topepo topepo commented Dec 15, 2017

I committed them to the fork pr/104 fork on the topepo/recipes repo. Here is a list of the changes.

[I find that committing to a PR to be confusing as hell. I never know if I should commit straight to your (forked) repo or to the PR on mine.]

EdwinTh added 2 commits Dec 15, 2017
@EdwinTh
Copy link
Contributor Author

@EdwinTh EdwinTh commented Dec 15, 2017

It is indeed quite a fuss, think we are all set now, but please do check...

@topepo
Copy link
Collaborator

@topepo topepo commented Dec 15, 2017

One more thing and I think we are there: please add a test file for the column check operation.

@EdwinTh
Copy link
Contributor Author

@EdwinTh EdwinTh commented Dec 16, 2017

I am learning something new about PRs every day. I did not know that when I pushed to my own github after doing a PR it was added to the PR automatically. Was not aware I already pushed the checkcolumns, it is unfinished indeed (not only tests are lacking).

@EdwinTh
Copy link
Contributor Author

@EdwinTh EdwinTh commented Dec 17, 2017

I think we are all set now.

@topepo
Copy link
Collaborator

@topepo topepo commented Dec 17, 2017

That warning is easy to fix so I'll get it.

Also, I'll merge in another PR after this that will add a new option called skip (just as an fyi).

Thanks!

@topepo topepo merged commit d3042ee into tidymodels:master Dec 17, 2017
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
topepo added a commit that referenced this pull request Dec 17, 2017
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.