-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Add linkcheck_case_sensitive configuration option #14046
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
Open
FazeelUsmani
wants to merge
42
commits into
sphinx-doc:master
Choose a base branch
from
FazeelUsmani:linkcheck_case_insensitive
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+185
−1
Open
Changes from all commits
Commits
Show all changes
42 commits
Select commit
Hold shift + click to select a range
63e108c
Update configuration.rst
FazeelUsmani caae7eb
Add linkcheck_ignore_case config option
FazeelUsmani 9e6dd40
Update i18n.py
FazeelUsmani eccd6d7
fixed the failing test test_numfig_disabled_warn
FazeelUsmani 6300483
Enable case-insensitive URL and anchor checking for linkcheck builder
FazeelUsmani b61366c
strip ANSI color codes from stderr before assertion
FazeelUsmani 7ea45c6
fixed the failing test test_connect_to_selfsigned_fails
FazeelUsmani 99a5dc0
Update test_build_linkcheck.py
FazeelUsmani f99651f
Merge branch 'master' into linkcheck_case_insensitive
FazeelUsmani ac12d63
Update linkcheck.py
FazeelUsmani 1a0d9ed
Update test_build_linkcheck.py
FazeelUsmani d115b1e
Update test_build_linkcheck.py
FazeelUsmani 0075419
fix ruff check linkcheck.py
FazeelUsmani 4eceef2
fix ruff check test_build_linkcheck.py
FazeelUsmani e772df9
Update configuration.rst
FazeelUsmani 14ded5b
Update configuration.rst
FazeelUsmani 386d4ac
Update configuration.rst
FazeelUsmani 53a47e3
Update doc/usage/configuration.rst
FazeelUsmani 3e545f3
Update i18n.py (reert \)
FazeelUsmani d9940da
Use .casefold() for case-insensitive URL comparison
FazeelUsmani 322fcf5
Update test_build_linkcheck.py (revert)
FazeelUsmani cfcbef2
Update test_build_linkcheck.py (revert)
FazeelUsmani 2c4567d
restore original pytest markers
FazeelUsmani c18d573
Removed the duplicate @pytest.mark.sphinx
FazeelUsmani 07b1795
Removed test_linkcheck_anchors_remain_case_sensitive
FazeelUsmani bc8fa7c
Rename linkcheck_ignore_case to linkcheck_case_insensitive and update…
FazeelUsmani 029a720
Fix ruff format check
FazeelUsmani 539adaa
remove unused code paths
FazeelUsmani ae5708f
Merge branch 'master' into linkcheck_case_insensitive
FazeelUsmani 66ae54d
Remove unused test parameter from numfig test
FazeelUsmani 5bc9f2d
Tests: Add complete coverage for linkcheck case sensitivity tests
FazeelUsmani eaa1caa
Refactor linkcheck case sensitivity: rename config and fix fragment h…
FazeelUsmani 57e8b3c
Improve formatting and update config value handling
FazeelUsmani 5dffff4
Update tests/test_builders/test_build_linkcheck.py
FazeelUsmani 5e08ab3
Remove deprecated linkcheck_case_insensitive config handling
FazeelUsmani 45cf720
Merge branch 'linkcheck_case_insensitive' of github.com:FazeelUsmani/…
FazeelUsmani 06663cf
Refactor linkcheck tests: rename handler for case sensitivity and sim…
FazeelUsmani 5615ffc
Add support for case-insensitive URL checking in linkcheck builder
FazeelUsmani 842b756
restore @pytest.mark.test_params and update documentation
FazeelUsmani 1fe4293
efactor linkcheck case sensitivity tests with dynamic path handler
FazeelUsmani 8c7648b
"Update test document with path1 and path2 for case sensitivity tests
FazeelUsmani d95224b
Apply ruff formatting
FazeelUsmani File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| # Empty config for linkcheck case sensitivity tests |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| `path1 <http://localhost:7777/path1>`_ | ||
|
|
||
| `path2 <http://localhost:7777/path2>`_ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional: I think it would be possible to merge the two test cases into a single
def test_linkcheck_case_sensitivitynow that we can pattern-match different URL patterns.(the code for the two of them is very similar currently, so perhaps the end result would be neater/smaller by combining them)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I explored merging the tests, but found that keeping them separate is actually clearer and simpler. Merging them would require either:
server port)
Since you mentioned this was optional, I've kept the two separate tests as they clearly demonstrate:
This makes the tests easier to understand and maintain. Please let me know if you still prefer them to be merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems reasonable to me, yep - thanks! One detail I'd like to ask about: with approach (2), was the more complex in the
CapitalisePathHandler? (that's what I'd guess, but would like to double-check)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, the complexity wasn't in the
CapitalisePathHandler- that part would stay the same (it just redirects/path→/Pathfor any request).The complexity would be in the test logic itself. To test both behaviors in one test, we'd need:
localhostand127.0.0.1)127.0.0.1)The issue is the RST document has hardcoded ports (e.g.,
http://localhost:7777/path), but the test server uses a dynamic port. So we'd need logic to construct the correct URLs with the dynamic port in the test assertions, which felt more complex than just having two focused tests.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok; yep, good observation about the dynamic port allocation for the test webserver vs the static port in the documentation.
And also point taken/agreed about the single
/path->/Pathtransform offered by the handler.We could add more path transforms to the handler.. could that help? (I think it might do, but I haven't experimented with it in code yet)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, adding more path transforms to the handler could definitely help!
Here's what I'm thinking:
Then the test RST could have:
path1 <http://localhost:7777/path1>_path2 <http://localhost:7777/path2>_And we configure the pattern to match only path1:
confoverrides={'linkcheck_case_insensitive': [r'http://localhost:\d+/path1']}
This way we can assert:
The dynamic port issue is actually not a problem for assertions since we use the actual address variable there. The hardcoded port in the RST only matters for the pattern matching, which this approach handles cleanly.
Want me to implement this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, ok! Let's try that, but with a change: I'm not too keen on the hardcoded redirect paths, so perhaps we could instead test for
self.path.islower(), and when it is, then redirect the client toself.path.capitalize(). I think that'd help to make the resulting code even more concise.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! I've implemented your suggestion with the dynamic path detection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thank you - there may be a couple of cleanups possible:
do_GETanddo_HEADimplemented again - I think we should apply the same simplification as previously and remove one of those.test_linkcheck_case_sensitivitythat handles both case sensitive and non-case-sensitive variants.I've realized that my suggestion to use
self.path.capitalize()isn't applicable as-is.. sorry about that - perhaps we could useself.path.upper()instead, though.