Add comments on ImageMagick requirement

[mxd4]

Subject: Add comments to clarify the cause of issue #10384

Purpose

Allow newcomers to quickly understand the requirement to ImageMagick if test_ext_imgconverter.py fails.

NB: First contribution, thanks for reviewing! :)

[mxd4]

CI failed on mypy and docslint steps.
I fixed the docslint error and rebased 5.x to get the mypy fix.
@tk0miya could you run it again? 🙏🏻

[mxd4]

Thanks a lot for the suggestions @AA-Turner
All done :)

[AA-Turner]

I think more usefully you should also change the warning

sphinx/sphinx/ext/imgconverter.py

Lines 32 to 36 in 8c7a472

	except OSError as exc:
	logger.warning(__('convert command %r cannot be run, '
	'check the image_converter setting: %s'),
	self.config.image_converter, exc)
	return False

Something like the following may be more useful:

-            logger.warning(__('convert command %r cannot be run, '
-                              'check the image_converter setting: %s'),
-                           self.config.image_converter, exc)
+            logger.warning(__(
+                f'Unable to run the image conversion command {self.config.image_converter!r}! '
+                'sphinx.ext.imgconverter' requires ImageMagick by defult. '
+                'Ensure it is installed, or set the configuration option '
+                "'image_converter' to a custom conversion command.\n\n"
+                f'Traceback: {exc}'
+            ))

A

[AA-Turner]

I'm somewhat conflicted on the change of the documentation -- as noted in the extension, ImageMagick is just a default, and can be changed. Perhaps add "by default" or similar?

sphinx/sphinx/ext/imgconverter.py

Lines 69 to 79 in 8c7a472

	if sys.platform == 'win32':
	# On Windows, we use Imagemagik v7 by default to avoid the trouble for
	# convert.exe bundled with Windows.
	app.add_config_value('image_converter', 'magick', 'env')
	app.add_config_value('image_converter_args', ['convert'], 'env')
	else:
	# On other platform, we use Imagemagick v6 by default. Especially,
	# Debian/Ubuntu are still based of v6. So we can't use "magick" command
	# for these platforms.
	app.add_config_value('image_converter', 'convert', 'env')
	app.add_config_value('image_converter_args', [], 'env')

A

[mxd4]

@AA-Turner I agree.
I've just added the suggested warning, as well as changes to the doc to make it clearer.
It's now explicit that the provided command is convert by default and uses ImageMagick, but can be overridden.

[AA-Turner]

Grammar suggestions, otherwise seems alright!

A

[tk0miya]

Thank you for your work, and sorry for the incovinience. I think it's better to skip the test if ImageMagick not found. So I just posted #10411 to do that.

[tk0miya]

LGTM with nits.

[mxd4]

No problem :) Thanks for reviewing!

[mxd4]

@tk0miya gentle ping, sorry for the noise. Is this okay to be merged? Thanks a lot 😃 🙏🏻

[tk0miya]

Sorry for my late response. Merging now. Thank you for your contribution!