altMITgcm / MITgcm

M.I.T general circulation model
MIT License
1 stars 7 forks source link

Code for MSE coordinate (first try with Github!) #10

Closed dfer22 closed 6 years ago

dfer22 commented 6 years ago

Test with github

dfer22 commented 6 years ago

I added MSE_LAYERS in LAYERS_OPTIONS.h. Not sure for your question about the header file. Glad you enjoyed Portugal!

christophernhill commented 6 years ago

@dfer22 I only see two files in the PR? https://github.com/altMITgcm/MITgcm/pull/10/files I don’t see LAYERS_OPTIONS.h

Did you commit, push etc... everything you meant to?

dfer22 commented 6 years ago

OK, I forgot that commiting takes more steps than "git commit". That said, I'm still a bit confused as I could not find the "Compare and pull" button anymore. Did you get a new pull request?

jklymak commented 6 years ago

@dfer22 You've already made the Pull Request. Any changes you git push origin back to GitHub on this branch bugfix_thebug will be part of the Pull Request.

Have a look at https://matthew-brett.github.io/pydagogue/gitwash_build.html for a guide to working w/ GitHub...

christophernhill commented 6 years ago

The PR gets updated when you push to your branch. In github the PR is just a way that you say I have this thing that I think is ready to merge, but the “thing” aka branch bugfix_thefix, is still only in your repo until it’s merged. I think that is right way to think of it?

jklymak commented 6 years ago

I think that’s right. The changes only live in the users repo until they are merged into the “upstream” repo.

dfer22 commented 6 years ago

Still not clear, this page: https://github.com/altMITgcm/MITgcm/blob/master/doc/contributing.rst seems to suggest a 2 step process (steps 6 and 7): first a "git push -u origin" (which, I understand, pushes the modifications to my personal branch), then a pull request on the Github website (to get these modifications to the master branch). Are you saying the second step is unnecessary?

jklymak commented 6 years ago

The PR is not necessary after its been done once.

A PR is saying to the upstream repository that you have a branch on your repository (bugfix_thebug) that you want to merge. You can keep making changes to that branch until its merged, but you don’t need to keep making a new pull request.

edoddridge commented 6 years ago

@dfer22 - the two steps required to add changes to a current pull request are 5 and 6:

(quoting some of step 7) "At last the maintainers will be notified and be able to peruse your changes! While the PR remains open, you can go back to step 5 and make additional edits, git adds, git commits, and then redo step 6; such changes will be added to the PR (and maintainers re-notified)."

dfer22 commented 6 years ago

OK, makes sense now. Might be worth to rephrase step 7 in a more transparent way (for slow people like me).

christophernhill commented 6 years ago

@dfer22 did you mean to close this PR (which you did) - or just close the comment thread?

The other thing it needs to be officially complete and get merged is an entry in doc/tag-index - not sure if that is in the instructions (so I am going to check).

dfer22 commented 6 years ago

Not sure, I don't remember closing anything. I'm still a bit lost with the github thing, so I may have done something wrong. What do I need to do to finish the thing off properly?

christophernhill commented 6 years ago

@dfer22 did you update doc/tag-index. I don't see an update in https://github.com/dfer22/MITgcm/blob/bugfix_thebug/doc/tag-index maybe you forgot to do git push?

e.g.

To finish the MSE PR it just needs you to add something like

_o Add moist-static energy mode (MSELAYERS) to package layers.

at start of doc/tag-index

We are hoping people will do that for all code mods, so doing it for this will be provide an example. That way we can improve use of doc/tag-index as a useful way to summarize what is changed/added/fixed and when.

christophernhill commented 6 years ago

+1 to merge. @jm-c ?

edoddridge commented 6 years ago

@christophernhill you've still got a review open with requested changes. One of us can dismiss it for you, but if you're happy it might be better to approve the changes.