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

Added argtypes and restype for MagickTransformImage #50

Merged
merged 5 commits into from Sep 6, 2012

Conversation

@mlindgren
Copy link
Contributor

@mlindgren mlindgren commented Sep 4, 2012

This change adds argtypes and a restype for MagickTransformImage, which allows that function to be used.

@travisbot
Copy link

@travisbot travisbot commented Sep 4, 2012

This pull request passes (merged 7c7ab64 into 7ad805f).

@dahlia
Copy link
Collaborator

@dahlia dahlia commented Sep 4, 2012

It should implement a higher-level interface like other operations, and tests for it as well. Could you implement these for us? :-) I will keep this pull request to go without closing.

@mlindgren
Copy link
Contributor Author

@mlindgren mlindgren commented Sep 4, 2012

Sure, I'll see what I can do. How do I run the tests? I'm not familiar with the testing system you're using.

@dahlia
Copy link
Collaborator

@dahlia dahlia commented Sep 5, 2012

You can run test using setup.py script:

$ python setup.py test
@@ -602,23 +602,23 @@ def format(self, fmt):
r = library.MagickSetImageFormat(self.wand, fmt.strip().upper())
if not r:
raise ValueError(repr(fmt) + ' is unsupported format')

This comment has been minimized.

@mlindgren

mlindgren Sep 5, 2012
Author Contributor

Cleaned up stray whitespace on this line as well as 611, 616, and 621. (Sorry to be pedantic... it shows up red because of how I have vim configured, so I try to get rid of it.)

@mlindgren
Copy link
Contributor Author

@mlindgren mlindgren commented Sep 5, 2012

Alright, added a higher level interface for the function via the Image.transform(...) method, and unit tests to verify that the function is correct. I also changed the minor version number; wasn't sure if you would have wanted that, so let me know if you'd like to change it back. I did my best to following the design, coding style and error handling conventions of the existing code, but let me know if there's anything else you'd like me to change in this commit.

Additionally, I can't easily use valgrind to verify that the C interface isn't leaking memory on the machine I'm on, so perhaps you could verify an assumption for me. Setting self.wand = new_wand (wand/image.py:1036) calls the Resource setter, thereby decrementing the reference count of the previous wand and destroying it if necessary, correct?

@travisbot
Copy link

@travisbot travisbot commented Sep 5, 2012

This pull request fails (merged 9f35e50 into 7ad805f).

@mlindgren
Copy link
Contributor Author

@mlindgren mlindgren commented Sep 5, 2012

Oops - looks like rounding behavior is machine-dependent when the dimensions of the image end up being a non-integer value (e.g. rounding a 402x599 image down to 50% of its original size); that caused the tests to pass on my machine but fail when Travisbot built them. I changed my unit test to use the 800x600 asset to avoid that issue.

@travisbot
Copy link

@travisbot travisbot commented Sep 5, 2012

This pull request passes (merged aa4a365 into 7ad805f).

@@ -30,7 +30,7 @@
#:
#: .. versionchanged:: 0.1.9
#: Becomes :class:`tuple`. (It was string before.)
VERSION_INFO = (0, 2, 2)
VERSION_INFO = (0, 2, 3)

This comment has been minimized.

@dahlia

dahlia Sep 5, 2012
Collaborator

We haven’t released 0.2.2, so you don’t have to change this. ;-)

the new image. By default is `True`.
:type reset_coords: :class:`bool`
.. versionadded:: 0.2.3

This comment has been minimized.

@dahlia

dahlia Sep 5, 2012
Collaborator

It should be 0.2.2 as well.

@dahlia
Copy link
Collaborator

@dahlia dahlia commented Sep 5, 2012

The code you made doesn’t have any responsibility to free the resource (self.wand here), so this patch is valid.

WIDTHxHEIGHT< Enlarges images with dimensions smaller than the
corresponding width and/or height dimension(s).
AREA@ Resize image to have the specified area in pixels.
Aspect ratio is preserved.

This comment has been minimized.

@dahlia

dahlia Sep 5, 2012
Collaborator

reStructuredText provides definition lists, so you can format this using that feature. Also, you can build the docs in local:

$ cd docs/
$ make html

and then built docs can be found in docs/_build/html/ directory.

… hasn't been released yet. Fixed formatting in transform function docstring so that it will render nicely with Sphinx, removed reference to parameter that no longer exists, and replaced raw URL with a proper seealso box. Also fixed a missing newline from master in ImageMagick Image Channel seealso which caused documentation to render incorrectly.
@mlindgren
Copy link
Contributor Author

@mlindgren mlindgren commented Sep 6, 2012

I reset the version to 2.2 and changed the docstring so that it renders nicely in my latest commit. Thanks for your guidance in helping me to make a documented and worthwhile contribution to this great project. :)

@dahlia
Copy link
Collaborator

@dahlia dahlia commented Sep 6, 2012

Nice work! :-) Thanks for your effort. It completely satisfies #30 so that we can close it.

dahlia added a commit that referenced this pull request Sep 6, 2012
Added argtypes and restype for MagickTransformImage. Fix #30
@dahlia dahlia merged commit 146e66c into emcconville:master Sep 6, 2012
1 check passed
1 check passed
@dahlia
default The Travis build passed
Details
dahlia added a commit that referenced this pull request Sep 6, 2012
dahlia added a commit that referenced this pull request Sep 6, 2012
@dahlia
Copy link
Collaborator

@dahlia dahlia commented Sep 6, 2012

I wrote the changelog for this: http://dahlia.kr/wand/changes.html#version-0-2-2.

@coveralls
Copy link

@coveralls coveralls commented Nov 20, 2013

Coverage Status

Changes Unknown when pulling f5059c4 on mlindgren:master into * on dahlia:master*.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants