I seem to remember the eval is there when you want to pass this parameter on the command-line via -D plot_gallery=0, I could be wrong about that.

I also seem to remember -D plot_gallery=False was not working last time I tried for some reason. Obviously if we can tidy this, it would be great.

I think both of those should work since there is the eval. At least I think I change it to plot_gallery=False and it worked okay

I never get this warning unless using 'make html-noplot'. Indeed it come from overriding the default values from the command line call of sphinx-build as it does not do a good job assigning types, it actually can't as command line input is strings.

For -D plot_gallery=0 or -D plot_gallery=False it works because because we have an eval down the code execution. But it does not work correctly for -D abort_on_example_error=False or with the zero because this option has no eval.

I don't like the idea of this PR of putting quoting the default value to avoid the warning. If this happen because we are overriding the configuration on the command line then is pretty sure the the person making the call knows what he is doing. On the contrary having the default value as a string looks band and I'm unsure if that will inspire confusion. For people willing to have a permanent change in their conf.py file they shall see that the defaults are boolean variables.

For me I'm fine with the warning. But I will indeed put an extra eval for the abort_on_example_error

I never get this warning unless using 'make html-noplot'.

Yes, we use this standard option a lot, both in CircleCI automated builds for PRs and ask contributors to do it locally because our full build takes forever.

For me I'm fine with the warning.

I don't think that packages (this one included) should emit warnings when functionality is used in standard, correct ways. It promotes ignoring warnings, which is bad practice.

having the default value as a string looks band and I'm unsure if that will inspire confusion.

Yeah, this the downside of this change. My understanding is that the tradeoff is essentially between having an effect on all users correctly using the package (a warning; current behavior) for an effect that is internal to sphinx-gallery code/maintainers (string default; proposed behavior) that runs a small risk of temporarily confusing devs -- although perhaps that could be fixed by an appropriate/better comment?

For people willing to have a permanent change in their conf.py file they shall see that the defaults are boolean variables.

How do you mean? To me this is just an internal aesthetic 'True' versus True distinction. Where would this permanent change come up?

How do you mean? To me this is just an internal aesthetic 'True' versus True distinction. Where would this permanent change come up?

With permanent change I mean defining your defaults. Sphinx-Gallery defaults to plot_gallery=True, but you can change this behaviour and set in your conf.py:

sphinx_gallery_conf ={
...
'plot_gallery':False}

In that way your default in not executing the examples and if that is your most used that may better be your default.
Then when building the gallery executing your examples, to have all the plots you can use an option make html-with-plots that you have in your make file to pass -D plot_gallery=True

For me I'm fine with the warning.
I don't think that packages (this one included) should emit warnings when functionality is used in standard, correct ways. It promotes ignoring warnings, which is bad practice.

I do agree on that, I did scan Sphinx code to see what it does to the config values, but I could not see a good way for it to take arguments of the command line with a type.

With permanent change I mean defining your defaults.

Ahh, so customize it with conf.py. Yes we could do this but I wouldn't want to -- it would lose easy transitions between packages, standard Makefile contents, and the like.

In any case I'm not sure how this ability to change the default affects whether or not we should change the default inside sphinx-gallery code. Users will never see the string value so shouldn't be confused by it, and like I said we can make it clear to devs.

I did scan Sphinx code to see what it does to the config values, but I could not see a good way for it to take arguments of the command line with a type.

So you're okay with this change, then? Should I improve the comment somehow?

ping @Titan-C any thoughts? I still get these warnings on every run.

This one is still good to go from my end

(if @Titan-C you are convinced)

@Eric89GXL how does this change what users need to do? At all?

how does this change what users need to do? At all?

It doesn't change anything at the user end, just makes it so a warning goes away that otherwise always shows up, 3rd line of any make html-noplot build in red:

WARNING: The config value `plot_gallery' has type `unicode', defaults to `bool'.

in that case I'm +1 on this. Maybe add a "users should still use True" in the comment so that it doesn't confuse folks that happen to stumble upon the code.

(but I'll defer to @Titan-C since y'all were discussing this prior to me jumping in)

I think that this is a case of "practicality beats purity". What are the drawbacks? This PR seems to fix the problem quite simply.

The only drawback I see is that devs might be confused why a string value is given instead of a standard bool, but there is a comment in the code for this reason.

@GaelVaroquaux done

@Titan-C : what do you think?

As there seems to be a consensus here, I am merging this. Thanks!