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

Remove doctype from resolver #5230

Merged
merged 4 commits into from Feb 28, 2019
Merged

Conversation

@stsewd
Copy link
Member

@stsewd stsewd commented Feb 5, 2019

Total explanation why this is safe to remove in:

#4638 (comment)
#4638 (comment)

Ref #4638

@stsewd stsewd force-pushed the remove-doctype-from-resolver branch from fc8d84c to 1c068f0 Feb 5, 2019
@stsewd stsewd requested review from ericholscher and Feb 5, 2019
@stsewd
Copy link
Member Author

@stsewd stsewd commented Feb 5, 2019

Also, our current logic is making wrong assumptions about mkdocs projects https://www.mkdocs.org/user-guide/configuration/#use_directory_urls

path = filename + '.html'
else:
path = ''
return path
Copy link
Member

@ericholscher ericholscher Feb 6, 2019

I do feel like we introduced this for a reason, but perhaps we've removed that path from the codebase or something. I agree that the less we do with guessing URL's for things, the better, but I could still see this breaking in a random place.

Copy link
Member Author

@stsewd stsewd Feb 6, 2019

So, what I was thinking, this is used to reverse the url results from search, so all urls are present in the same format as the docs are build, rather than always including .html/index.html

Copy link
Member Author

@stsewd stsewd Feb 6, 2019

That's the only place where I can think it could break something, but when indexing from search we save the whole file, with the extension, no need to reverse anything from there. The other places we really don't use this at all.

Copy link
Member

@ericholscher ericholscher Feb 12, 2019

I'm still a little worried that this might break things, but I don't have a good way of testing it beyond just deploying it. @agjohnson do you have any memory of why we added this?

@stsewd stsewd requested a review from agjohnson Feb 18, 2019
Copy link
Member

@ericholscher ericholscher left a comment

@stsewd if you feel like this has been tested enough, I'm fine merging it and seeing if anything blows up in prod :)

@stsewd
Copy link
Member Author

@stsewd stsewd commented Feb 27, 2019

Ok, couldn't find anything breaking. Just more bugs related to the doctype thing that are present in master :)

@ericholscher can you approve this again to merge?

@stsewd stsewd requested a review from ericholscher Feb 27, 2019
@stsewd stsewd merged commit 4737c83 into readthedocs:master Feb 28, 2019
1 check passed
@stsewd stsewd deleted the remove-doctype-from-resolver branch Feb 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants