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

Moved GmDisplayViewer into API Additions #2

Closed
wants to merge 2 commits into from

Conversation

radarhere
Copy link

Helps python-pillow#5349

This PR moves your documentation from "Other Changes" into "API Additions", because, well, it is technically an API addition.

You will then realise that in 8.2.0, we've actually already added one other new ImageShow.Viewer subclass - IPythonViewer (python-pillow#5289). So this PR also updates your text so that they are more similar to each other. Feel free to tell me that you think your version is better worded in parts.

I'm also correcting

If both are installed, the test suite prefers ImageMagick over GraphicsMagick.

It's not just the test suite that prefers ImageMagick. im.show() does as well.

See https://pillow-radarhere.readthedocs.io/en/graphicsmagick/releasenotes/8.2.0.html#api-additions for how it looks.

@radarhere radarhere changed the title Moved GmDisplayViewer into API Additions [ci skip] Moved GmDisplayViewer into API Additions Mar 22, 2021
@latosha-maltba
Copy link
Owner

latosha-maltba commented Mar 23, 2021

Thanks for the input.

This PR moves your documentation from "Other Changes" into "API Additions", because, well, it is technically an API addition.

Can't argue with that. Remark: I now realise that we have slightly different priorities. For me its all about running the test suite with GraphicsMagick (that viewer addition just being an easy bonus because I spotted it), while you are more focused on the Pillow user side, i.e. the viewer and API, (thus running the test suite is a bonus).

Anyway, I've taken your suggested but re-added the entry about running the test suite with GraphicsMagick under "Other Changes". I also expanded a little about

If both ImageMagick and GraphicsMagick are installed, ImageMagick will be used.

Since the previous paragraph did not mention the ImageMagick viewer, the above wording sounds (at least to me) as if the newly added class (the GraphicsMagick-Viewer) falls back to ImageMagick if both are installed.

That said, we now have 3 commits solely about the wording of the release notes, so I squashed them into one commit (on the master branch) giving you credit with a

Co-authored-by: Andrew Murray <radarhere@users.noreply.github.com>

line. If you prefer other ways, just tell me and I'll make it happen.

@radarhere
Copy link
Author

I'm curious - why is the test suite a priority for you? Particularly this situation, where only two tests are affected, and from what I see in the code, they wouldn't even fail without either ImageMagick or GraphicsMagick.

Oh, and don't worry about ensuring that I have credit - I'm a frequent Pillow contributor, I am not lacking in commits credited to me.

@latosha-maltba
Copy link
Owner

Maybe "priority" was a too strong word. It's not a priority in the sense that some project/job depends on it (I'll program only for fun in my leisure time). I have a laptop which runs GraphicsMagick (but not ImageMagick) and every time I encounter a package which uses ImageMagick, I think, why not try to get support for GrahpicsMagick, too.

In this instance I found (more or less by accident) that some tests depended on ImageMagick, so I set my goal as "fix that test suite". Since Pillow was only a dependency I only built it and ran the test suite but did not use it. That's why my main concern was the test suite and not that additional viewer. Usually (as in this case) the actual changes are trivial, so I try to push them upstream, so someone with a more constrained working environment can benefit from it.

Lastly, I'd want to say thanks again for the help and the pull requests provided! It was a pleasure working with you.

@radarhere radarhere deleted the graphicsmagick branch September 20, 2023 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants