-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Show suggestion on the 404 page #678
Conversation
This feature would be really cool, and I really like how you've enumerated all the different cases, and figured out what type of message to show for each one. I can see that you've put a lot of work into this. However, there's a lot of duplicated work here. Inside your new So, how can we use the work that's already been done? Here a different approach:
Lastly, as you demonstrate, there are a lot of cases to consider, so it would be great to include some tests to show that they work as expected. Please don't consider any of the above to be the final word! I'm just throwing out my ideas in the interest of starting a discussion to figure out the best way to make this feature work. |
Thank you for your comment.
I Agree. They should have been done somewhere else. I tried getting slugs somehow not from
Absolutely. I'll add tests before refining the implementation. It might take some time to learn how to use Django's test framework. Also, your approach makes sense. It seems good to me. Current implementation is rough so any ideas are welcome, and I'm sure it needs to be refine. But if there is no big problem, I think done is better than perfect. Once it has been merged, anybody can refine it. (I will try, of course, but it's slower than Django users do.) |
Please take another look. |
Thanks for updating so quickly...and especially, for including tests! I'll take a look at this tomorrow. |
I think this is a good start, but I don't think it's quite ready for merging. The goal is to display helpful links on the 404 page, so users can find the content they are looking for. But, as it works now, there are a number of cases where the links presented are for invalid URLs that link to another 404 page. Here are a few examples:
https://pip.readthedocs.org/en/nonexistent/ -> displays 'latest' https://pip.readthedocs.org/en/nonexistent/foo.html -> displays 'latest' https://pip.readthedocs.org/fr/latest/foo.html -> displays 'en' It's a real challenge to match filenames, since they aren't stored in the DB and so the Django app has no knowledge of what HTML files exist for each set of documentation. So, if you are searching for 'install.html' for a particular version of a project and it doesn't exist, what's the best way to show a list of versions where 'install.html' does exist? I'm honestly not sure. Is the list of files easily accessible from Elasticsearch? Could we use that? Do we scan the filesystem before generating the 404 to compile the list of files? (That sounds bad...) Do we store the list of files in the DB somehow? (Keeping the DB and filesystem in sync sounds bad...) I'm open to suggestions. Lastly, I should note that the text of the error messages and the template would need some refining before launching, but that's easily doable once the feature works well. |
Yes, I recognize the issues and it would be nice if they are solved. Also, nobody can add tests for cases you've found and refine text of the error messages until this change is merged. That means I need to do all things discussed here. I'm not going to do that, especially finding files things. So my suggestion is that merge this change to some branch and keep refining on RTD's repository, and discuss for improvement on new issue. IMO, making language switching better is more useful for users than keep discussing and making helpful 404 page better. |
I mean, finding files feature will take much time. I have no idea to achieve this. |
@maskit Thanks for hanging in there and continuing to work on the PR. I like your idea to pull the branch into the RTD repository, and then let's see if we can merge some of it piecemeal. Hopefully, we can get a basic helpful 404 message for non-existent languages and versions first, and try to tackle the filename matching as a second step. Do you want to tackle the 1st issue above before I create the branch on the RTD repo? |
Ping @ericholscher @robhudson. I've got a couple of elasticsearch questions... Is there a way to query for a list of HTML documentation filenames for a given project? If someone navigated to If this query isn't currently possible, given what we are indexing, is this a type of problem for which elasticsearch would be a good solution? If so, how would we do it? Lastly, I'm noticing other situations where having a list of documentation filenames for each project/version would be handy. For instance, generating a sitemap (issue #557) would require knowing every doc filename for a project. My other ideas for coming up a list of filenames are either to store the filenames in the DB, or to walk the filesystem. Perhaps, elasticsearch is a better option? Your thoughts on the best way to handle this? |
We have the language. version. and page indexed. So all we need to do is parse that out of that URL, which we should easily be able to do. The filesystem is the canonical place for that data though, so depending on the use case it seems like we should look at the file system. Definitely for the site map generation, as it should be part of the build. We can also guarantee that a page will not 404 if it exists on the file system, where as keeping it in sink in elastic search will be harder.
|
When you say the language. version. and page are "indexed", you mean in elasticsearch, right? |
Yep. It is definitely doable with ES. I'm on my phone at the moment though, so I can't say much more until later.
|
@toffer Thank you for your understanding.
Yes, that is what I wanted to say. There is no need to do them in one step.
Does "the 1st issue" mean |
Yes. That's what I meant.
Perfect. |
Conflicts: readthedocs/templates/404.html
I figured out the 1st issue above. Actually it's not a helpful 404's issue but doc_url tag's. I added tests for that. |
Thanks for the update. I'll take a look at this in the next few days. |
Went ahead and merged this, because it is an improvement over what we currently have. We should open a new ticket about improvements. |
Show suggestion when users land on the 404 page.
What to show are commented in the source code.