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

Fix for units argument. Adds a res argument. #2617

Merged
merged 1 commit into from Jan 8, 2013

Conversation

milkypostman
Copy link

Update the units argument so that it accepts values from ["px", "in",
"cm", "mm"].

The res argument was added for the cases where units is specified
and is not default (px). A default value of 72 is provided by rmagic in
the case that units is not "px" but res is not specified.

Update the `units` argument so that it accepts values from ["px", "in",
"cm", "mm"].

The `res` argument was added for the cases where `units` is specified
and is not default (px). A default value of 72 is provided by rmagic in
the case that `units` is not "px" but `res` is not specified.
@milkypostman
Copy link
Author

This is a fix for #2615

@takluyver
Copy link
Member

Thanks, that looks promising. Have you tested the various different options?

I'll let someone more familiar with this code check it as well.

@milkypostman
Copy link
Author

I've tested this with units in and using different res values. I was wondering if there is some test code somewhere I should write?

@takluyver
Copy link
Member

That's an excellent idea. Have a look at the tests here:

https://github.com/ipython/ipython/blob/master/IPython/extensions/tests/test_rmagic.py

On 25 November 2012 15:28, Donald Ephraim Curtis
notifications@github.comwrote:

I've tested this with units in and using different res values. I was
wondering if there is some test code somewhere I should write?


Reply to this email directly or view it on GitHubhttps://github.com//pull/2617#issuecomment-10694554.

@takluyver takluyver merged commit 14aacee into ipython:master Jan 8, 2013
@takluyver
Copy link
Member

Thanks again. I added a bit to the test and merged this.

@milkypostman
Copy link
Author

Sorry I didn't get to this.

On Jan 7, 2013, at 18:36, Thomas Kluyver notifications@github.com wrote:

Thanks again. I added a bit to the test and merged this.


Reply to this email directly or view it on GitHub.

@takluyver
Copy link
Member

That's OK. Thanks for making the fix, that's the important bit :)

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

2 participants