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

Fixed a bug in offsetbox #1859

Closed
wants to merge 5 commits into from
Closed

Fixed a bug in offsetbox #1859

wants to merge 5 commits into from

Conversation

jrevans
Copy link

@jrevans jrevans commented Mar 26, 2013

Offsetbox was using a local member before it was set. This has been corrected.

James Evans added 5 commits November 28, 2011 11:36
Merging from my 'working' branch to my 'master' branch?
Merging Working back into Master
* 'master' of github.com:jrevans/matplotlib:
* upstream/master: (545 commits)
  compat.subprocess: Turn on absolute importing.
  api_changes: Document the API changes.
  Moved matplotlib.subprocess_fixed to matplotlib.compat.subprocess.
  cbook.report_memory: If ps not found, raises NotImplementedError, not OSError.
  texmanager: Ignore dvipng if it cannot be found, instead of raising OSError.
  subprocess_fixed: Gracefully handle platforms without subprocess.Popen.
  All modules now use matplotlib.subprocess_fixed instead of subprocess.
  Added new module, matplotlib.subprocess_fixed, as a replacement for subprocess.
  backend_ps: Do not write to a temporary file unless using an external distiller.
  DOC improvements on scatter plot
  MEP10 - documentation improvements on many common plots: scatter plots, imshow, matshow etc
  finance: Use context manager to close files.
  finance.fetch_historical_yahoo: Create directories leading up to cachename.
  BF - correct return type for Axes.get_title
  DOC axes now refers to the Markers module instead of concatenating docstrings
  DOC improvements on the markers module
  MEP10 - documentation improvements on the markers module
  PEP8 fixes on the markers module
  Improving triinterp_demo pylab example
  Improved test_triinterp_colinear.
  ...
@dmcdougall
Copy link
Member

+1.

Can you squash this into one atomic commit, though?

@jrevans
Copy link
Author

jrevans commented Mar 26, 2013

I am not sure what you mean by "squash this into one atomic commit". I have only made the one change.

@dmcdougall
Copy link
Member

@jrevans There are extra commits loitering about. They are 0cfcea9, 9a9bd20, 00f55eb, and a44f340. You can get rid of them by doing the following (assuming you're currently on the master branch and your most recent commit is ece229c): git -i rebase HEAD~5.

An editor will pop up with instructions on how to 'fixup' your commits. You can reorder them, too.

If you're not comfortable doing this I can do it locally and merge it for you.

@efiring
Copy link
Member

efiring commented Mar 27, 2013

@jrevans, your PR includes 5 commits, but it should include only the last. The others are merge commits that are not relevant and would clutter the history. Maybe the problem arose because you did not make your change in a topic branch relative to up-to-date master. http://matplotlib.org/devel/gitwash/development_workflow.html

Is the bug you are fixing in 1.2.x? If so, the fix should go there first.

@dmcdougall
Copy link
Member

@efiring I was just looking into the 1.2.x branch for this and, yes, the bug exists there too.

@dmcdougall
Copy link
Member

Sorry, my mistake. It's fine in 1.2.x. That's weird. Why didn't it get merged?

You can see it here: https://github.com/matplotlib/matplotlib/blob/v1.2.x/lib/matplotlib/offsetbox.py#L1451

@dmcdougall
Copy link
Member

The offending commit is: dd32575.

So, in short, there's no need to apply this to the 1.2.x branch. Sorry for the spam.

Edit: And a link for the lazy.

@pelson
Copy link
Member

pelson commented Mar 27, 2013

Presumably this is not used anywhere in the mpl codebase (and is certainly not tested).
It begs the question, can we simply delete the class? Is there anybody using this functionality? If we want to keep it, is there a test which we can add for this?

@WeatherGod
Copy link
Member

It is used for making a dragable legend.

@pelson
Copy link
Member

pelson commented Apr 18, 2013

I've cherry picked the last commit onto master (2920408). Thanks @jrevans.

@pelson pelson closed this Apr 18, 2013
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.

None yet

5 participants