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

autodetect ImageMagick on windows #1041

Merged
merged 11 commits into from
May 12, 2020
Merged

autodetect ImageMagick on windows #1041

merged 11 commits into from
May 12, 2020

Conversation

Nex4rius
Copy link
Contributor

@Nex4rius Nex4rius commented Nov 27, 2019

Autodetect ImageMagick on windows

@coveralls
Copy link

coveralls commented Nov 27, 2019

Coverage Status

Coverage decreased (-0.09%) to 67.306% when pulling 0560b70 on Nex4rius:patch-1 into 427c122 on Zulko:master.

moviepy/config.py Outdated Show resolved Hide resolved
@tburrows13
Copy link
Collaborator

Would someone else like to approve this PR?

@tburrows13 tburrows13 added the LGTM Passed inspection by maintainers. label Feb 23, 2020
@tburrows13 tburrows13 added this to the Release v1.0.2 milestone Feb 23, 2020
@tburrows13 tburrows13 added the lib-ImageMagick Issues pertaining to dependency ImageMagick. label Feb 24, 2020
moviepy/config.py Outdated Show resolved Hide resolved
moviepy/config.py Outdated Show resolved Hide resolved
@tburrows13 tburrows13 removed the LGTM Passed inspection by maintainers. label Apr 14, 2020
@tburrows13
Copy link
Collaborator

Thanks for the contribution. However, I think that this has been superseded by #1109.

@Nex4rius
Copy link
Contributor Author

Nex4rius commented Apr 14, 2020

I have to disagree.

I looked at #1109 and with that it checks for the ImageMagick environment variable and then uses that.
I just looked and after installing ImageMagick it didn't set the environment variable which means I still have to manually add it.

I still think it's better if it can autodetect it without configuring / setting environmental variables / doing anything.

Also this does no harm and is literally the last fallback if everything else fails.

@tburrows13
Copy link
Collaborator

Sorry, I think you're right, I had possibly misinterpreted #1109. I agree that this does additional processing that #1109 does not do.

You now need to run black over it with black -t py36 .. I'd also suggest using encoding="utf-8" in check_output instead of decoding the output.

set encoding directly
@Nex4rius
Copy link
Contributor Author

Nex4rius commented Apr 15, 2020

I added the encoding but what is that black -t py36 command?

Isn't that like less readable?

@tburrows13
Copy link
Collaborator

tburrows13 commented Apr 16, 2020

In cases like this, yes, it is arguably less useful, but overall it ensures that the style is consistent which is a great benefit (it was very bad before we started using it!) .

You can install it with pip install black, and then run black -t py36 . from the root directory of MoviePy. Thanks!

@tburrows13
Copy link
Collaborator

Hey, thanks for getting it sorted with black! Its a big help to have the code all formatted consistently (I know that yours was fine (just different in matters of taste) before, but a lot of contributions aren't and it is so much easier to run a single tool on a submission than it is to nitpick every little change.) For future reference it should be a one-line command and commit, it looks like you tried to do it manually? Either way, it's fine now.

I made a couple more smaller changes that I just noticed. @mgaitan was right about not using a bare except: https://stackoverflow.com/questions/54948548/what-is-wrong-with-using-a-bare-except. We don't want to catch things like KeyboardInterrupt or SystemExit.
And your code was giving warnings about invalid escape characters because it used backslashes in the string without marking it as raw with the r before it.

If you're happy with my changes, then I'm happy to merge!

@Nex4rius
Copy link
Contributor Author

Sure looks fine now :)

@Nex4rius
Copy link
Contributor Author

What exactly are we waiting for until merge?

@tburrows13
Copy link
Collaborator

Nothing, just all the maintainers are quite busy :) Commenting reminders every so often like you've done is a good idea.

@tburrows13 tburrows13 added the feature New addition to the API i.e. a new class, method or parameter. label May 12, 2020
@tburrows13 tburrows13 merged commit 4d70eab into Zulko:master May 12, 2020
@tburrows13
Copy link
Collaborator

Are you using image magick 6 or 7? I notice that this uses convert.exe which I think is IM6 only? Is there a reason why it shouldn't be changed to magick.exe for IM7?

@Nex4rius
Copy link
Contributor Author

I have imagemagick 7 and it has both convert.exe and magick.exe

@Nex4rius Nex4rius deleted the patch-1 branch May 23, 2020 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New addition to the API i.e. a new class, method or parameter. lib-ImageMagick Issues pertaining to dependency ImageMagick.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants