Skip to content

Makes eventplot legend work #7676

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

Merged
merged 2 commits into from
Dec 31, 2016
Merged

Conversation

nepix32
Copy link
Contributor

@nepix32 nepix32 commented Dec 23, 2016

Fixes issue #7662

@tacaswell tacaswell added this to the 2.0.1 (next bug fix release) milestone Dec 23, 2016
@tacaswell
Copy link
Member

Can you also add a test to https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/tests/test_axes.py ?

something like

@cleanup
def test_eventplot_legend():
    # your code snippet from the bug report

@QuLogic
Copy link
Member

QuLogic commented Dec 26, 2016

Can you rebase and squash the first two commits together?

@@ -4871,3 +4871,8 @@ def test_scatter_color_masking():
assert_array_equal(facecolors[1], np.array([0, 0, 0, 1]))
assert_array_equal(linecolors[1], np.array([0, 0, 0, 1]))
assert linewidths[1] == 3

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more blank line.

@QuLogic
Copy link
Member

QuLogic commented Dec 27, 2016

When you rebase, you need to force push (git push --force) and not pull again or you'll get two copies of your commits (new from you and old from GitHub).

@codecov-io
Copy link

codecov-io commented Dec 27, 2016

Current coverage is 62.11% (diff: 100%)

Merging #7676 into master will increase coverage by 0.03%

@@             master      #7676   diff @@
==========================================
  Files           174        174          
  Lines         56021      56021          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          34773      34795    +22   
+ Misses        21248      21226    -22   
  Partials          0          0          

Powered by Codecov. Last update ab98852...ad74d47

@nepix32
Copy link
Contributor Author

nepix32 commented Dec 27, 2016

Think this all went horrible wrong (I hate now git to its guts), how to best clean up this mess:

  • Delete this pull request
  • Create a new one (with only the useful changes from above)
  • Rebase if necessary

@QuLogic
Copy link
Member

QuLogic commented Dec 27, 2016

A new PR isn't necessary; just a rebase (and force push) will do. I can do that for you (you will need to reset your local copy.)

@nepix32
Copy link
Contributor Author

nepix32 commented Dec 27, 2016

I would say I have done enough damage already and ask you kindly to do the rebase and force/push.

@QuLogic
Copy link
Member

QuLogic commented Dec 27, 2016

BTW, your name on commits is sometimes nepix32 and sometimes your email; what is the correct name you wish to use? (configure this with git config --global user.name 'The name to use', but it only affects new commits.)

@nepix32
Copy link
Contributor Author

nepix32 commented Dec 27, 2016

nepix32 shall be user.name, thanks for notice. Should be configured properly now.

@QuLogic
Copy link
Member

QuLogic commented Dec 27, 2016

I updated the PR; on your computer,

$ git status
# Check for any changes you want to save (commit to a branch or stash them)
$ git fetch origin
$ git checkout nepix32-patch-1
$ git reset --hard origin/nepix32-patch-1

@nepix32
Copy link
Contributor Author

nepix32 commented Dec 27, 2016

Have done everything down to git reset ... and things look properly now locally. What is the correct way now to synchronize my local copy with the fork of matplotlib without breaking everything again? git push?

@QuLogic
Copy link
Member

QuLogic commented Dec 27, 2016

I replaced the branch on your GitHub fork and the commands I gave above reset your local copy to match the fork. Everything should be in sync (as far as this PR/branch is concerned.)

@nepix32
Copy link
Contributor Author

nepix32 commented Dec 27, 2016

Ok, I see, was looking at the forks master. In ...patch... everything is fine. What do you recommend to reset the master in the fork to look exactly as the one in upstream?

@QuLogic
Copy link
Member

QuLogic commented Dec 27, 2016

$ git checkout master
$ git reset --hard upstream/master
$ git push --force origin master

should get your local copy of master in sync with the version of here that your local copy believes to be the latest and sync your fork with that.

In the future, (or now just to be sure you have the latest commit)

$ git checkout master
$ git fetch upstream
$ git merge upstream/master
$ git push origin master

should be a fast-forward merge that just moves master "up" to the latest commit.

@nepix32
Copy link
Contributor Author

nepix32 commented Dec 27, 2016

Hmmm... still have changes in origin/master I cannot get rid of going back to a commit before. But anyway, many thanks for helping me to deal with git!

@QuLogic
Copy link
Member

QuLogic commented Dec 27, 2016

Feel free to stop by our Gitter room for some help.

@QuLogic QuLogic requested a review from tacaswell December 29, 2016 06:51
@tacaswell
Copy link
Member

Looks good to me.

And adds tests to previously untested code path!

@NelleV NelleV changed the title Makes eventplot legend work [MRG+1] Makes eventplot legend work Dec 30, 2016
@NelleV
Copy link
Member

NelleV commented Dec 31, 2016

Thanks for the patch!

@NelleV NelleV merged commit 159f36e into matplotlib:master Dec 31, 2016
@NelleV NelleV mentioned this pull request Dec 31, 2016
@QuLogic QuLogic modified the milestones: 2.0 (style change major release), 2.0.1 (next bug fix release) Jan 1, 2017
@QuLogic QuLogic changed the title [MRG+1] Makes eventplot legend work Makes eventplot legend work Jan 1, 2017
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.

5 participants