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

Codify linkcheck status codes into a Literal #13040

Merged
merged 4 commits into from
Oct 19, 2024

Conversation

AA-Turner
Copy link
Member

Alternative to #13039. This changes nothing in the runtime other than narrowing the current str to a Literal. Note that Literal[''] is needed in a couple of places because of this.

cc @jayaddison

A

@jayaddison
Copy link
Contributor

Alternative to #13039. This changes nothing in the runtime other than narrowing the current str to a Literal. Note that Literal[''] is needed in a couple of places because of this.

Yep, sounds good. What do you think about introducing an unknown status to initialize to instead of '' in a subsequent change? In practice I don't think it should ever not be assigned to a valid value, but empty-string does look a bit quirky (again, agreed with the refactor-only idea though).

@AA-Turner AA-Turner merged commit d773a43 into sphinx-doc:master Oct 19, 2024
23 checks passed
@AA-Turner AA-Turner deleted the linkcheck-codes-literal branch October 19, 2024 19:44
@AA-Turner
Copy link
Member Author

AA-Turner commented Oct 19, 2024

Thanks!

What do you think about introducing an unknown status to initialize to instead of '' in a subsequent change?

In practice, '' is a logic error. It is only reached if retries is 0, which is an illegal value (though seemingly allowed at present). The logic is to return the first non-failing result or the final result if all fail. (Edit: but adding 'unknown' sounds good!)

A

Copy link
Contributor

@jayaddison jayaddison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it 👍

As an aside: I think I may have misunderstood the incompatibility of Literal with StrEnum - perhaps both can be combined effectively here after all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants