X Tutup
The Wayback Machine - https://web.archive.org/web/20210120012957/https://github.com/mkdocs/mkdocs/pull/2202
Skip to content

/ mkdocs

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

Jump to bottom

Fix url-filter with file urls on an empty site_url, closes #2189 #2202

Open
wants to merge 4 commits into
base: master
from
Open

Fix url-filter with file urls on an empty site_url, closes #2189 #2202

wants to merge 4 commits into from

Conversation

@ktomk
Copy link

@ktomk ktomk commented Oct 12, 2020

Previously when "use_directory_urls" was false and "site_url" empty (e.g.
documentation for fs), the link to the home-page / main index was broken
missing /index.html at the end of the url.

Fix is to add the missing index.html in such a case by the url-template-
filter (... | url).

NOTE: Some values to the filter are declared undefined behavior as the
filter will return wrong urls. These cases are considered theoretical
(a file-name must end with a dot and be at the end of value) and not
likely to happen.
Declaring them undefined now, allows to change the implementation
later in a compatible way.
@ktomk ktomk force-pushed the ktomk:fix-missing-index-on-empty-site-url branch from 3d2f6b6 to 507ca07 Oct 12, 2020
ktomk added a commit to ktomk/mkdocs-material that referenced this pull request Oct 12, 2020
@ktomk
Fix-up site_url (link behind home button) for usage from file://
Loading status checks…
ea1332c 
Fix in 454339b only works on base directory level, pages in subdirectories
were still broken.

Similar patch for url filter in mkdocs/mkdocs#2202.

Ref: 454339b
@ktomk ktomk mentioned this pull request Oct 12, 2020
Closed
@ktomk ktomk force-pushed the ktomk:fix-missing-index-on-empty-site-url branch 2 times, most recently from 676984e to ff0c93c Oct 12, 2020
@waylan
Copy link
Member

@waylan waylan commented Oct 27, 2020

This should include a test which fails before the change and passes with the change.
@ktomk ktomk force-pushed the ktomk:fix-missing-index-on-empty-site-url branch from ff0c93c to 7a82a09 Oct 28, 2020
@ktomk
Copy link
Author

@ktomk ktomk commented Oct 28, 2020

@waylan If you can take a quick look, I've reset the PR branch to a single commit capturing the current behavior for the url-filter in a test. Please check if it aligns with your expectations.

If everything is fine, I would add more commits:

  • first change the test so that it tests for the correct behavior to add the index.html if a link to a directory index and use_directory_urls is false. the test-suite then should be red.
  • second a commit with the fix. the test-suite then should become green again.

The second commit changes would be the same as already proposed (507ca07 - might be temporarily go away on Github due to the refresh of the PR), this is only to introduce the tests you've asked for.
@waylan
Copy link
Member

@waylan waylan commented Oct 28, 2020 •
edited

As you likely noted, no existing filter tests exist. This is because the one filter we have simply wraps the mkdocs.utils.normalize_url function and we have tests for that in utils_tests.py. I would expect any changes to be made to the mkdocs.utils.normalize_url function and and new tests to be added in utils_tests.py.

Finally, I did not expect you to provide the tests without the fix. When I stated that the test should fail before the fix is applied, I simply meant that the test should fail if the fix was removed, not that you provide only the tests first. Sorry if I wasn't clear.
@ktomk
Copy link
Author

@ktomk ktomk commented Oct 29, 2020

@waylan Could you add more reference to the normalize_url test in utils_tests.py you've commented ("we have tests for that in utils_tests.py"), I can not find it.

And no worries, having the tests first is just an intermediate to clarify things upfront, never did any pull request to mkdocs so just stepping in.
@waylan
Copy link
Member

@waylan waylan commented Oct 29, 2020

Maybe I should have said that we should have tests for that in utils_tests.py. If they are not there (as appears to be the case), then we should add some. Although, some of the functions in utils call other functions in utils. Sometimes you will find the tests are on the outer function and not on the inner one. That might be the case here (I havn't checked).

In any event, at a minimum I would expect the tests added in this PR to be in utils_tests.py and test the normalize_url function directly. If you want to add any additional tests, that's fine, but without those minimum tests I won't be merging this.
@waylan
Copy link
Member

@waylan waylan commented Oct 29, 2020

I'm assuming this is intended to be a fix for #2189. Adding the link here for later reference.
@ktomk
Copy link
Author

@ktomk ktomk commented Oct 29, 2020

@waylan Ah okay, now I get it. Let me check what I can add to it, might be that the function is not the right one to patch, as effectively the filter is needed (would be the outer function then). #2189 is good to have referenced, Github would do this automatically as it's in the commit message of the fix.

So quick question, would it be acceptable if the test is for the filter function (explicitly), to have it also in the (proper?) filters_tests.py file? - OR - do you want to have normalize_url function be patched (adding another parameter, not in my original fix suggestion) and then this all goes into utils_tests.py file.

If I have the answer to this I should be able to prepare the PR in full.
@waylan
Copy link
Member

@waylan waylan commented Oct 30, 2020

normalize_url is called in other places, not just from the filter. Is the fix unique to the filter? If not, then the fix needs to go in normalize_url and the tests need to be for that function. However if the fix is unique to the filter and the issue will never occur elsewhere, then it would make more sense for the fix to go in the filter and the tests to be against the filter.
@ktomk
Copy link
Author

@ktomk ktomk commented Oct 30, 2020

Yes, the fix is unique to the filter. This is why I created the file to host filter tests. Thanks for clarifying.

The reason why I created tests upfront is just to capture the existing behavior, so the scope of change is more visible. Now seeing how this might came a bit as of a surprise especially at that place.

With the discussion so far, I think it is worth to move fixtures to a test of the inner normalize_url function to create it in the utilities to have it covered, as no test exists yet for it and it is at least within the context of the change.

Looking forward to refresh the PR next.
@ktomk ktomk force-pushed the ktomk:fix-missing-index-on-empty-site-url branch from 7a82a09 to d015930 Oct 30, 2020
ktomk added 3 commits Oct 30, 2020
@ktomk
Fix flaky test_get_themes()
daf552a 
Some environments have more themes installed which is not an error.

Therefore assertion against the expected as subset, not super-set.
@ktomk
Capture normalize_url and get_relative_url behavior
3ca8141 
Add tests for normalize_url and get_relative_url utiliy functions.
@ktomk
Capture url filter behavior
e34969d 
In a new unit-test case via fixtures and matrix of base_url and
use_directory_urls:

- base_url:
  - empty
  - not empty
- use_directory_urls:
  - false
  - true
@ktomk ktomk force-pushed the ktomk:fix-missing-index-on-empty-site-url branch from d015930 to aeab718 Oct 30, 2020
@ktomk
Fix url-filter with file urls on an empty site_url, closes #2189
Loading status checks…
1c3f6bd 
Previously when "use_directory_urls" was false and "site_url" empty (e.g.
documentation for fs), the link to the home-page / main index was broken
missing /index.html at the end of the url.

Fix is to add the missing index.html in such a case by the url-template-
filter ( {{ ... | url }} ).

NOTE: Some values to the filter are undefined behavior as the filter
      will return wrong urls. These cases are considered theoretical
      (a file-name must end with a dot and be at the end of value) and not
      likely to happen.
      Declaring them undefined now, allows to change the implementation
      later in a compatible way.
@ktomk ktomk force-pushed the ktomk:fix-missing-index-on-empty-site-url branch from aeab718 to 1c3f6bd Oct 31, 2020
@ktomk
Copy link
Author

@ktomk ktomk commented Oct 31, 2020

@waylan could you add a bit more context about the design decision? Just seeing you added a tag and it would help me to learn more what that relates about.
@waylan
Copy link
Member

@waylan waylan commented Oct 31, 2020

The tag is just a reminder to myself. At a glance it appears that you've provided a reasonable patch. When I have more time I need to review it in the context of the discussion in #2189. I've removed the [update needed] tag, so I'm not expecting any more work at this time.
@ktomk
Copy link
Author

@ktomk ktomk commented Nov 1, 2020

Please take whatever time you need to take.

I do not see any design decision blocking the merge (it's just a non-design fix), nevertheless take your time with the merge as well.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Needs design decision
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
@ktomk @waylan
You can’t perform that action at this time.
X Tutup