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

Adding support for query strings in the edit_uri #1224

Merged
merged 1 commit into from May 13, 2017

Conversation

Projects
None yet
2 participants
@ovasquez
Contributor

ovasquez commented May 10, 2017

Fixing #1220
Includes tests and documentation.

@waylan

Thanks for your work on this. I made a few inline comments. Also, note that you have a few failing tests regarding formatting: flake8 and markdown-lint. At the very least those would need to be fixed before this can be accepted.

new_url = urlunparse(url_parsed)
new_path = urlunparse(path_parsed)
return urljoin(new_url, new_path)

This comment has been minimized.

@waylan

waylan May 10, 2017

Member

This seems like an awful lot of code just to support query string URLs. Is this all really necessary? I'm not even convinced we need to use urljoin. If we do, fine, but if not then removing that use seems like a better option.

Also, if we explicitly support query strings, then we should explicitly support hash urls as well (http://example.com/path/to/root/#/foo/bar/page.md).

@ovasquez

This comment has been minimized.

Contributor

ovasquez commented May 10, 2017

@waylan Thanks for all the comments. Regarding the use of urljoin and the complex code, it might end up being easier to restrict the use of query strings and hash urls only in the edit_uri, that way the first and straightforward solution of concatenating the edit_uri and the self.input_path would work.

Do you consider this restriction viable for the end user? If yes, then it will be simple to support and to document, as there will not be any complex behavior involved.

@waylan

This comment has been minimized.

Member

waylan commented May 10, 2017

self.input_path is always going to be the local path from docs_dir to the local file. Therefore, it would never contain a query string or hash. In fact, it is not even overridable by the user. The two unknowns are the repo_url and edit_uri. The user could put anything in either config setting. We validate repo_url to check that urlparse can parse it and that it contains a scheme, but that's it. edit_uri could be any string. Although, if the repo_url points to a known host (only GitHub and Bitbucket at this point), then we do auto-populate edit_uri (if it is unset). I suspect the current implementation only expects URLs of the style used by those two services.

So to answer your question, I would expect the only location of a query string or hash to be in the edit_uri, but I suppose one could be found in the repo_url (although it seems highly unlikely to me).

@ovasquez

This comment has been minimized.

Contributor

ovasquez commented May 10, 2017

Based on the discussion, I will add support for query and hash URLs in the edit_uri, which is a lot better than no support at all.

I believe it will cover almost all cases and can implemented without any complex logic. I'll update the documentation to clarify this. Thanks for the help!

@ovasquez ovasquez force-pushed the ovasquez:master branch 2 times, most recently from fee15c0 to a68b9b4 May 12, 2017

@ovasquez

This comment has been minimized.

Contributor

ovasquez commented May 12, 2017

@waylan I updated the code with a much simpler version, tests covering more cases and improved documentation. The windows to posix path change in the code is the simplest version I could find that worked in python 2.6-2.7 without going completely overkill. I didn't use os.path.sep (like in get_url_path) because I needed the Windows tests to pass in Linux.

Regarding of whether or not to include the '/' before the query or fragment character, after reading and searching for a while I understood that both (with or without) options are allowed by modern browsers so restricting to one style might prevent some users from using this functionality. Here's an interesting read about the topic http://stackoverflow.com/a/42193734

@waylan

Looking good. I see only two additional tests that should be added, one of which may reveal a small bug. Overall, I'm much more comfortable with this.

# Ensure the '/' is added to the repo_url for simple edit_uri
repo_url = 'http://example.com'
edit_uri = 'edit/master/docs/'

This comment has been minimized.

@waylan

waylan May 12, 2017

Member

Perhaps we should also include a test where the edit_uri is missing the slash at the end.

This comment has been minimized.

@ovasquez

ovasquez May 12, 2017

Contributor

Sure, I will include those tests later today. I think I'll just create 2 test functions, one containing all tests for posix-style paths and the other one for windows-style paths, to reduce the length of the code.

if not edit_uri:
self.edit_url = repo_url
else:
if not edit_uri.endswith('/'):
edit_uri += '/'
# Normalize URL from Windows path '\\' -> '/'
input_path_url = self.input_path.replace('\\', '/')

This comment has been minimized.

@waylan

waylan May 12, 2017

Member

You might need to do this before added a missing slash (switch order of this and previous statement). As is, it could result in two slashes in some circumstances. Probably should add a Windows specific test for that as well.

Also, any reason why you didn't use mkdocs.utils.path_to_url() here?

This comment has been minimized.

@ovasquez

ovasquez May 12, 2017

Contributor

Sounds good, I'll update the code to prevent this issue.

And not using mkdocs.utils.path_to_url() because it uses pathname2url which will turn the '?' and '#' to their encoded versions and generate wrong URLs.

This comment has been minimized.

@waylan

waylan May 12, 2017

Member

Ahh right. Just wanted to make sure there was a good reason.

@ovasquez ovasquez force-pushed the ovasquez:master branch from a68b9b4 to bb956ec May 13, 2017

@ovasquez

This comment has been minimized.

Contributor

ovasquez commented May 13, 2017

@waylan I updated the code and tests as requested. Let me know what you think.

@waylan

waylan approved these changes May 13, 2017

@waylan waylan merged commit aa9a2fd into mkdocs:master May 13, 2017

3 checks passed

codecov/project 93.76% (target 90%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment