sphinx icon indicating copy to clipboard operation
sphinx copied to clipboard

[8.x] deprecate intersphinx format

Open picnixz opened this issue 1 year ago • 1 comments

Extracted from #11706.

cc @jayaddison

picnixz avatar Mar 14 '24 09:03 picnixz

Have rebased this on the now-split intersphinx package.

A

AA-Turner avatar Apr 25 '24 15:04 AA-Turner

Do we need to update ENV_VERSION with this change?

jayaddison avatar Jul 22 '24 11:07 jayaddison

Do we need to update ENV_VERSION with this change?

I don't know but to be sure, let's do it :)

picnixz avatar Jul 22 '24 11:07 picnixz

Do we need to update ENV_VERSION with this change?

We shouldn't need to, as the resulting data that gets stored in the environment is unchanged -- we haven't changed how we do the normalisation.

A

AA-Turner avatar Jul 22 '24 11:07 AA-Turner

Actually, by doing so, it would at least invalidate the cache (which could contain old intersphinx references). But if you want, you can revert the last commit (just tell me whether it's you or me that need to work on the branch now; I don't want race issues)

picnixz avatar Jul 22 '24 11:07 picnixz

just tell me whether it's you or me that need to work on the branch now

I'm done for now, don't worry

AA-Turner avatar Jul 22 '24 11:07 AA-Turner

@jayaddison

Do we need to update ENV_VERSION with this change?

@AA-Turner

We shouldn't need to, as the resulting data that gets stored in the environment is unchanged -- we haven't changed how we do the normalisation.

@picnixz

Actually, by doing so, it would at least invalidate the cache (which could contain old intersphinx references). But if you want, you can revert the last commit (just tell me whether it's you or me that need to work on the branch now; I don't want race issues)

Ok. Perhaps reverting it here, but adding it in #12087 could make sense? Reasoning: it could be unexpected to have resolved non-deterministic inventories, and yet not rewrite existing environment files affected by that after an upgrade to 8.x takes place.

jayaddison avatar Jul 22 '24 11:07 jayaddison

Ok. Perhaps reverting it here

Actually, by invalidating the environment, you somewhat fix the bugs in #12087. I prefer erring on the safe side at the cost of a single rebuild (also, it's kind of a major change where now we issue errors instead).

Also, we are just storing less information so technically I would say we should still create a new environment (from a typing PoV, it's no more possible to have a None name, now it should always be a string).

picnixz avatar Jul 22 '24 11:07 picnixz

@picnixz do you have any other changes, or is this ready to merge from your perspective?

A

AA-Turner avatar Jul 22 '24 11:07 AA-Turner

do you have any other changes, or is this ready to merge from your perspective?

I don't have anything else! I expect people to be surprised if they build very old projects but I think the test coverage is sufficient. I'm just unsure whether the typing definitions I put should be kept in shared or not but this is something we can decide later (for now, let's keep them like that because the other PR is based on this one and I don't want to again solve conflicts). By the way, thank you for the CHANGES entry.

picnixz avatar Jul 22 '24 11:07 picnixz

Thanks all!

A

AA-Turner avatar Jul 22 '24 12:07 AA-Turner

Hi!

I just stumbled onto this PR as the root cause of our docs breaking. While I looked back and there was a deprecation warning cast by Sphinx 7.0 (which is nice, thanks!), the error message in 8.0 could be a bit clearer, pointing directly to some docs or migration guide on how to update it.

ERROR: Invalid value `None` in intersphinx_mapping['http://docs.python.org/']. Expected a two-element tuple or list.

For anyone also stumbling on this PR, the our fix was replacing:

intersphinx_mapping = {"http://docs.python.org/": None}

with

intersphinx_mapping = {'python': ('https://docs.python.org/3', None)}

As following these docs.

I was a bit amazed to see a 10 year old line of code breaking our docs configuration. Doesn't happen often!

EwoutH avatar Aug 12 '24 10:08 EwoutH

Thanks @EwoutH. Do you have any concrete suggestions for the error message? I'm happy to update it.

A

AA-Turner avatar Aug 14 '24 23:08 AA-Turner