FIX: use warning instead of errors for version json tests

[12rambau]

Fix #987

I didn't removed the tests but changed their behaviors.
Now if the file is not found/reachable it will just throw a warning in the build log.

I test the key only if the file can be read and it will also only send a warning in the build log.

The only step that will still raise an error (and block the build) is if the file is not json parsable.

@tomboehling could you test your build process against this PR ?

[choldgraf]

This looks good to me - anything else to finish here?

[coretl]

Personally I'd still like to see an environment variable added so this checking could be disabled entirely. There was a discussion on this over on the related issue #987

[12rambau]

@coretl I added your suggestion to the code, could you try it against your build ?
@choldgraf as there's update in the docs since your approval can I request your review again ?

[choldgraf]

This looks good to me - my one comment is whether we should include the extra kwarg as another optional field of the switcher configuration, rather than creating a new option just for the switcher. What do you think? Not a huge deal either way though

[coretl]

@coretl I added your suggestion to the code, could you try it against your build ?

Works for me, thanks

https://github.com/coretl/switcher-test/actions/runs/3393550773/jobs/5641013851

[choldgraf]

Is this one ready to go?

[12rambau]

For me yes I think I addressed everyone's comments

[ClementPinard]

Hello,

I feel like the json check should not be by default, so that already existing pipelines which rely on a not already reachable version switcher won't have to be updated.

As such, the check_switcher option would be more about debugging than normal behaviour.

Just muy two cents as I would definitely prefer not to change all the conf.py files of all my company's private libs 😂

[choldgraf]

I'm a bit confused @ClementPinard - if the switcher JSON isn't reachable then isn't it in a broken state?

Can we think of cases where a person wants the behavior that their JSON isn't reachable but they want the test to pass anyway? If that is the case then this would be a breaking change for them...

[ClementPinard]

my proposition was just to set check_switcher to False by default.

That way, if you want to test the json, you just need to switch it to True, which means by default the old behaviour is kept (with possible errors in the json that remain undetected), but you can have the test if you explicitly mention it in the option

It all comes down to what we want to enforce between tests or no tests by default, We are at 0.x after all so a breaking change is understandable. I would advocate to keep the no test policy by default (so that I can upgrade my pydata version without having to change the conf.py ), but wanting to test by default is understandable as well.

Can we think of cases where a person wants the behavior that their JSON isn't reachable but they want the test to pass anyway? If that is the case then this would be a breaking change for them...

Why would it be a breaking change for them if no test is performed as before ?

[choldgraf]

Why would it be a breaking change for them if no test is performed as before

I mean that from your perspective it is a breaking change, because now your docs wouldn't build.

my proposition was just to set check_switcher to False by default

I'm fine w/ that - we could always set it to true in a future PR if somebody wished for it, and treat it as a deprecation cycle or something

[ClementPinard]

I mean that from your perspective it is a breaking change, because now your docs wouldn't build.

Ah sorry, now I understand what you were saying.

The related issues describes a few examples :

You have implemented a new check that fails if the connection to the file is not possible.

There could be many reasons for a connection error:

• Restricted access to the url (not possible in pipeline)
• first run of pipeline
• file is created later in the pipeline

My case is the last one. During the docs building CI, if no version_switcher is found in the url, it is created along the docs, and then everything gets uploaded at the same time. So it is normal that the first build is not working

[12rambau]

I decided to set the test to Trueby default because of the following scenario:

You are a newcomer to the lib. There are a lot of parameters you need to understand to build your application correctly, including the switcher one. Writing the .json file by hand, it's easy to make a mistake (forgetting a , typo in the url etc...). If the test is not performed by default, you need to identify that something went wrong with your .json and realize that the test option exists. I think people will miss it.

On the other hand, there are bigger projects with running CI that already master the theme, I think they will understand where the error is coming from (the very existence of this PR shows it) and can afford to add 1 extra parameter in the conf.py along with the version change PR.

Is it making sense ?

[drammock]

I find @12rambau'S reasoning convincing; new users will benefit from the test being active by default. @ClementPinard would it make your life any easier if it were controlled by an environment variable instead of a conf.py setting?

[choldgraf]

I also like the general principle of "if you're going to cause confusion, give it to the people that know what they're doing rather than newcomers to the theme". I know that is inconvenient for folks in your position though @ClementPinard :-/

[ClementPinard]

@ClementPinard would it make your life any easier if it were controlled by an environment variable instead of a conf.py setting?

Might be interesting, but then it would be weird having one option decided by the conf.py + an environment variable and the rest only by the conf.py. Still an interesting feature idea, to let all the theme options be either specified in the conf.py or in then environment ! As a matter of fact, I am already using environment variable, but get them back in the conf.py with os.getenv so would probably make my code smaller 😄

As said before, although breaking, this change is ok since we are not at 1.0.0 yet. I took the decision to use this lib even though there's no stable version yet, so it's not a big deal to have to change the conf.py in my repos.

Go for it 👍

[12rambau]

Another argument in favor of the configuration parameter is portability. Setting up an environment variable just for building a doc could be a mess if you have 2 documentation one with test included and the other without.

That being said I really like this idea:

let all the theme options be either specified in the conf.py or in then environment

If everyone is happy I think it's ready to be merged !

[drammock]

it would be weird having one option decided by the conf.py + an environment variable and the rest only by the conf.py.

to me the justification for doing this one variable differently is that the version switcher creates / faces problems that other files don't have, i.e., it's the only theme-integral file that can (or maybe even should) be outside the build tree; there are browser CORS (cross-domain JSON request policy) issues to deal with (which might behave differently on local builds, CI servers, and deploy servers), etc.

I'm actually -1 on supporting all config values to be in the environment or conf.py, seems burdensome to maintain and we're a bit short-handed as it is.

For now, I'll go ahead and merge this, but I think we should consider maybe coming back to the envvar idea for this at some point.