Skip to content

Added code references to search results. #1947

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

Merged
merged 1 commit into from
Mar 4, 2025

Conversation

sarahboyce
Copy link
Contributor

@sarahboyce sarahboyce commented Feb 13, 2025

Description

Recently, we added a scroll to text fragment to improve the search result navigation experience.
This takes the first line that was found in the body and scrolls you to this line.

This PR adds references to our Python class/class method/function definitions which will appear as a sub-item of the page search result if they match the search.
This means that when someone searches for "select" they have choice as to what to navigate to. They can chose the "QuerySet.select_related" or "QuerySet.select_for_update". The anchors of these are also robust and so you will navigate to the definition of these (the scroll for text logic is a little fragile due to the html stripping logic of full text search).

search for "select"

This "choice" is even more useful when searching for something like "field":

search for "field"


Issues aiming to fix

Ideally this along with the previous highlight text change fixes #1649 and fixes #1734

Review notes

Note that I have added some folks as possible reviewers.
I am not 100% sure the review process given the new website WG but would appreciate feedback as I'd love to land the PR 🫶

This PR has 2 commits, the first commit is a refactor commit to split the test file into multiple test files, the second is the feature.

I originally used regex to full out these code references. @bmispelon rightly pointed out this is brittle. I have since written a custom Sphinx builder in order to use Sphinx's own functionality for getting the python objects.

@sarahboyce sarahboyce force-pushed the code-links branch 3 times, most recently from 0576631 to 511a787 Compare February 14, 2025 10:41
@sarahboyce sarahboyce marked this pull request as ready for review February 14, 2025 10:45
@sarahboyce sarahboyce force-pushed the code-links branch 10 times, most recently from 58cb36f to c0e3136 Compare February 17, 2025 13:38
@alexgmin
Copy link
Contributor

Is there a case in which it makes sense to have the first line of the code reference?
Based on my searches it always seems redundant with the second line
image

Everything else looks good to me.

@sarahboyce
Copy link
Contributor Author

Is there a case in which it makes sense to have the first line of the code reference?

It's true that it's duplicate information.
I personally thought reading QuerySet.select_related is nicer than reading django.db.models.query.QuerySet.select_related, but still kept the full path as it's interesting/useful 🤷

@sarahboyce
Copy link
Contributor Author

I could make it like:

QuerySet.select_related
django.db.models.query

@sarahboyce sarahboyce force-pushed the code-links branch 3 times, most recently from d6d9929 to e6a2f3b Compare February 20, 2025 08:24
@alexgmin
Copy link
Contributor

alexgmin commented Feb 20, 2025

It's true that it's duplicate information. I personally thought reading QuerySet.select_related is nicer than reading django.db.models.query.QuerySet.select_related, but still kept the full path as it's interesting/useful 🤷

It is, and I would prefer to keep the short version, but I don't know if there's a case in which the short version is not unique.

I could make it like:

QuerySet.select_related
django.db.models.query

This could also work better than the current one if only the shortened version is not viable due to uniqueness.

@sarahboyce
Copy link
Contributor Author

It is, and I would prefer to keep the short version, but I don't know if there's a case in which the short version is not unique.

Field for example, django.db.models.fields.Field and django.forms.fields.Field

@sarahboyce sarahboyce force-pushed the code-links branch 2 times, most recently from 8e90640 to 34fe8a2 Compare February 22, 2025 07:48
@sarahboyce sarahboyce requested a review from adamzap February 22, 2025 07:53
@sarahboyce
Copy link
Contributor Author

Could we merge the test module restructuring first? It seems like a good change, and it would make reviewing this easier.

#1965

@sarahboyce sarahboyce force-pushed the code-links branch 2 times, most recently from c52d91c to 5427398 Compare February 27, 2025 08:35
Copy link
Member

@bmispelon bmispelon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is shaping up to be amazing, I can't wait for it to be running in production.

It looks great overall, I haven't found anything that seems wrong. I've left a few ideas for improvements, mostly around readability and trying to name things in a way that helps understand what's going on. Don't hesitate to push back on my suggestions if you think they don't make sense to you: at this point you're the domain expert here.

At the risk of repeating myself: amazing work! ✨

@bmispelon
Copy link
Member

Could we merge the test module restructuring first? It seems like a good change, and it would make reviewing this easier.

#1965

I took the liberty of merging that PR and rebasing + force-pushing this one. It made my reviewing easier and I hope I haven't messed up your local branch too hard 😅

@sarahboyce sarahboyce force-pushed the code-links branch 4 times, most recently from 9c98c6c to 7296712 Compare February 28, 2025 08:21
@sarahboyce sarahboyce requested a review from bmispelon February 28, 2025 08:24
@sarahboyce sarahboyce force-pushed the code-links branch 3 times, most recently from a5e66ef to 2554cf7 Compare February 28, 2025 09:57
Copy link
Member

@bmispelon bmispelon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the confoverrides["extensions"] is not needed (see comment), otherwise I think this is ready to ship as far as I'm concerned 👍🏻

@sarahboyce sarahboyce force-pushed the code-links branch 2 times, most recently from a13f1f1 to 96589bf Compare March 1, 2025 07:43
Copy link
Member

@bmispelon bmispelon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The double instantiating of Sphinx() was clever! I've made an alternate proposal that uses sphinx.config.Config() rather than a whole Sphinx application.

I also noticed that the multi-cpu processing had been lost, so I left a suggestion for how to re-add it. I think the logic around the conditional verbosity has also not been carried over, but I don't think that's a huge deal.

We're almost there! 😁

Thank you Baptiste Mispelon for the review.
Copy link
Member

@bmispelon bmispelon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic work!

From my perspective it's ready to deploy, but I'll probably wait from an official 👍🏻 from the website WG before I do so.

@sabderemane sabderemane merged commit fa56d9b into django:main Mar 4, 2025
4 checks passed
alexgmin added a commit to alexgmin/djangoproject.com that referenced this pull request Mar 5, 2025
bmispelon pushed a commit that referenced this pull request Mar 5, 2025
bmispelon added a commit to bmispelon/djangoproject.com that referenced this pull request Mar 11, 2025
bmispelon added a commit that referenced this pull request Mar 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants