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

pp.py - add new example of string comparison #3068

Closed
wants to merge 3 commits into from

Conversation

nhsavage
Copy link
Contributor

Relates to issue #3063 - adds one extra example to doctests for iris.fileformats.pp.STASH

@SciTools-assistant SciTools-assistant added the Blocked: CLA needed See https://scitools.org.uk. Submit the form at: https://scitools.org.uk/cla/v4/form label Jun 15, 2018
@nhsavage
Copy link
Contributor Author

I'll get on to the CLA ASAP. I also need to fix the actual doctest I added - I blindly copied and pasted code from issue.
I need to work out how to run doctests myself from this Windows box....

@pelson
Copy link
Member

pelson commented Jun 18, 2018

First things first: Congratulations on your first Iris PR (pull request)! 🎉 🍻 🎉

With regards to the CLA, I've just added you to the list at SciTools/scitools.org.uk#208. We should find that percolate through to the CLA checker in the next hour or so (so that we get a green tick, rather than a scary red cross next to the SCiTools-CLA-checker. ❌ ➡️ ✔️

Finally, regarding the change itself: looks great! It may make sense for us to target the v2.1.x branch, rather than master, that way we would get this in the v2.1.1 release if we chose to make one. It is a relatively painless process (with luck) on your side if you want to give it a shot, otherwise I can do it from my side:

# Make sure you actually have a remote called upstream...
git fetch upstream

# Take the 2 commits you've made, and apply them to a different "base"
git rebase upstream/v2.1.x

# That magic line changed the position of the commits in the Iris repository,
# which is the same as changing the history (for your 2 commits).
# We therefore need to "force" push the changes to your GitHub remote.
git push -f origin stasheq_doc

That is all that is needed on the git front. On the GitHub side of things, hit "edit" on this PR, and change the base branch to v2.1.x instead of master.

Don't worry if you mess any of this up, I've taken a copy of your PR just in case it goes wrong, and I'm on hand if you need a pointer. 😉

@SciTools-assistant SciTools-assistant removed the Blocked: CLA needed See https://scitools.org.uk. Submit the form at: https://scitools.org.uk/cla/v4/form label Jun 18, 2018
@nhsavage
Copy link
Contributor Author

nhsavage commented Jun 18, 2018 via email

@nhsavage
Copy link
Contributor Author

I think I have done the right thing, although I'm having problems trying to work out if I have.

@pelson pelson changed the base branch from master to v2.1.x June 19, 2018 08:39
@pelson pelson changed the base branch from v2.1.x to master June 19, 2018 08:39
@pelson
Copy link
Member

pelson commented Jun 19, 2018

I think I have done the right thing, although I'm having problems trying to work out if I have.

Not quite I don't think. Did you git rebase upstream/v2.1.x? It looks like you might have done git rebase upstream/master accidentally?

@nhsavage
Copy link
Contributor Author

hmm. I thought I had copied and pasted the command you sent but possibly not. I won't have time to look at this tonight though as it seems quite fiddly to undo a rebase. How do I check what I actually did and how can I test the rebase before pushing?

@nhsavage
Copy link
Contributor Author

would it be easier to make a new branch from upstream/v2.1.x instead of master?

@ajdawson ajdawson changed the base branch from master to v2.1.x June 19, 2018 18:07
@pelson
Copy link
Member

pelson commented Jun 20, 2018

I've just merged this in #3072. Thanks for persevering @nhsavage, and congratulations on your first commit! 👍

@pelson pelson closed this Jun 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants