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

update CWG2R based on suggestions by @mmlopezu #132

Merged
merged 7 commits into from
Mar 4, 2016

Conversation

zkamvar
Copy link
Contributor

@zkamvar zkamvar commented Feb 5, 2016

I've added comments to let the user know to start on step 3 if they've already forked and cloned the repository.

Question: should I change the steps to:

  • STEP i. Fork
  • STEP ii. Clone
  • STEP 1. Update
  • STEP 2. Branch
  • STEP 3. Commit

I think it might be easier this way.

sessionInfo:
R version 3.2.3 (2015-12-10)
Platform: x86_64-apple-darwin13.4.0 (64-bit)
Running under: OS X 10.11.3 (El Capitan)

locale:
[1] en_US.UTF-8/en_US.UTF-8/en_US.UTF-8/C/en_US.UTF-8/en_US.UTF-8

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

other attached packages:
[1] git2r_0.13.1

loaded via a namespace (and not attached):
[1] htmltools_0.3   tools_3.2.3     yaml_2.1.13     memoise_1.0.0   rmarkdown_0.9.2
[6] knitr_1.12.3    digest_0.6.9    devtools_1.10.0
@mmlopezu
Copy link

mmlopezu commented Feb 5, 2016

👍

@smanel
Copy link
Contributor

smanel commented Feb 7, 2016

YES!

zkamvar referenced this pull request Feb 9, 2016
This isn't ready... images might need to be added
@smanel
Copy link
Contributor

smanel commented Feb 9, 2016

Yes...If I understand you have not made the change now as suggested above?
If you make the change, you will need to change the 6 below steps?

Perhaps finally better to keep the 6 below steps:

1 Fork the popgenInfo repository*
2 Clone your fork to your local machine.*
3 Checkout your master branch, fetch the changes from NESCent and merge them into your fork.
4 Create a new branch and then add and commit new changes or content.
5 Push your changes to your fork.
6 Open a pull request from your fork to NESCent.

At the moment you have 5 steps in the vignette and there is finally no section 6. It is easy to create one: just before mgs<-

Finally Why not adding this as an additional step?
7-Pull request from your github fork Nescent -->and you can provide some explanations on how to do?
"Once you are finished with all of your changes, you can create a pull request from your GitHub fork to NESCent"

@zkamvar
Copy link
Contributor Author

zkamvar commented Feb 9, 2016

@smanel,

Yes...If I understand you have not made the change now as suggested above?

if you look at the commits, I have indeed made the changes as suggested above in c62b06b.

At the moment you have 5 steps in the vignette and there is finally no section 6. It is easy to create one: just before mgs<-

I saw your comment and have started to make changes in 688403e. I'm waiting on @hlapp's answer for #134 to include images.

Finally Why not adding this as an additional step?
7-Pull request from your github fork Nescent -->and you can provide some explanations on how to do?
"Once you are finished with all of your changes, you can create a pull request from your GitHub fork to NESCent"

This is actually redundant with Step 6.


The final order is:

i. Fork
ii. Clone

  1. Update master
  2. Checkout new branch from master
  3. Add, commit, and push
  4. Make a pull request

The last one is a bit difficult to write as github has no concrete guides that I can point to for this kind of workflow.

@zkamvar
Copy link
Contributor Author

zkamvar commented Feb 9, 2016

Note, you can see all of the changes I make by clicking here: https://github.com/NESCent/popgenInfo/pull/132/files

@smanel
Copy link
Contributor

smanel commented Feb 10, 2016

Sorry! I was looking the bad file.

Concerning the last section (I think that this is what I need to do for my vignette), I suggest to develop

"Once your pull request passes peer review and is published, you can update your
+fork by starting from Step 1: checkout your master branch, fetch the changes
+from NESCent, merge those changes, REVISED YOUR VIGNETTE, and push (STEP 3) them up to the master branch on your
+fork to make it even with NESCent." FINALLY CREATE THE PULL REQUEST (STEP 4)

Is it correct? If yes I will try to apply it .

@zkamvar
Copy link
Contributor Author

zkamvar commented Feb 10, 2016

"Once your pull request passes peer review and is published, you can update your
+fork by starting from Step 1: checkout your master branch, fetch the changes
+from NESCent, merge those changes, REVISED YOUR VIGNETTE, and push (STEP 3) them up to the master branch on your
+fork to make it even with NESCent." FINALLY CREATE THE PULL REQUEST (STEP 4)

Is it correct? If yes I will try to apply it .

Hi @smanel,

In your case, you should continue to add and commit changes to your vignette and push those changes since your pull request (#121) is still open and has not been published.

I now realize where the confusion lies. You are currently in step 4, but there are no instructions on what to do if changes need to be made (which is to continue adding content, committing, and pushing). I will add these instructions today.

@zkamvar
Copy link
Contributor Author

zkamvar commented Feb 10, 2016

@smanel, in other words, no, you do not need to update your branch at this time.

@smanel
Copy link
Contributor

smanel commented Feb 10, 2016

I use the following R script. Now what is missing?

library(git2r)
repo <- repository()
checkout(repo, "26_01_16_Signal_Selection")
repo
status(repo)
add(repo, "*") # adding all new or changed files
status(repo)
msg <- " review signal selection vignette: correction boxplot"
commit(repo, msg, session = TRUE)
status(repo) # should return nothing
repo
summary(commits(repo)[[1]])
push(repo, credentials = cred_token())

@zkamvar
Copy link
Contributor Author

zkamvar commented Feb 10, 2016

I use the following R script. Now what is missing?

library(git2r)
repo <- repository()
checkout(repo, "26_01_16_Signal_Selection")
repo
status(repo)
add(repo, "*") # adding all new or changed files
status(repo)
msg <- " review signal selection vignette: correction boxplot"
commit(repo, msg, session = TRUE)
status(repo) # should return nothing
repo
summary(commits(repo)[[1]])
push(repo, credentials = cred_token())

Nothing is missing! 😺

@zkamvar
Copy link
Contributor Author

zkamvar commented Feb 11, 2016

@smanel, I have made changes to include instructions on peer review. Are these sufficient?

@smanel
Copy link
Contributor

smanel commented Feb 12, 2016

I think yes. But I probably need to read all the vignette again and not only short revised sections to have a global idea of the vignette. Is it possible?

@zkamvar
Copy link
Contributor Author

zkamvar commented Feb 12, 2016

You can download it here. I've rendered it on my machine and placed it in a zip file:
CWG2R.zip

@smanel
Copy link
Contributor

smanel commented Feb 14, 2016

A long vignette!
I will test it again with a new vignette (I hope soon).

@zkamvar
Copy link
Contributor Author

zkamvar commented Feb 14, 2016

Yeah, that's what ends up happening with how-to guides. 😐

Sent from my iPhone

On Feb 14, 2016, at 12:41, Stéphanie Manel [email protected] wrote:

A long vignette!

I will test it again with a new vignette (I hope soon).


Reply to this email directly or view it on GitHub.

@zkamvar
Copy link
Contributor Author

zkamvar commented Feb 15, 2016

If it helps, steps 1-4 in the vignette correspond to steps 1-4 in the figure: https://github.com/NESCent/popgenInfo/pull/143/files

@zkamvar
Copy link
Contributor Author

zkamvar commented Mar 3, 2016

@smanel, do you think this version is good enough to merge at the moment? I would argue that it's much improved over the version that exists now.

@smanel
Copy link
Contributor

smanel commented Mar 4, 2016

Where I am sure to read the last changes?
In comment #153? In this case, yes I think that it can be merge.

@zkamvar
Copy link
Contributor Author

zkamvar commented Mar 4, 2016

@smanel, #153 is suggesting to change CONTRIBUTING.md, BUT it is a summary of the same flow that CONTRIBUTING_WITH_GIT2R.Rmd has. Given that this is the case, I'll merge it. If you find any problems, please create an issue and I will fix them.

zkamvar added a commit that referenced this pull request Mar 4, 2016
update CWG2R based on suggestions by @mmlopezu
@zkamvar zkamvar merged commit 6faa990 into NESCent:master Mar 4, 2016
@zkamvar zkamvar mentioned this pull request Mar 4, 2016
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.

3 participants