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

Combine results with AND #2514

Merged
merged 6 commits into from
Jun 14, 2024
Merged

Conversation

aaruni96
Copy link
Contributor

This makes the search combine input with AND instead of the default OR, which means results get more refined the longer your search string is, not more varied.

@mortenpi
Copy link
Member

This seems reasonable to me, but cc @Hetarth02 for a second opinion.

@Hetarth02
Copy link
Contributor

It looks good to me. But before merging some examples displaying how it has improved search results should be nice.

@aaruni96
Copy link
Contributor Author

Here's the search in the documentation for Oscar.jl using old behaviour (using "OR" to combine)

image

Here's the same query on the same set of documentation using AND to combine

image

200+ results v/s 35 results

@fingolfin
Copy link
Contributor

The search results @aaruni96 showed were from https://docs.oscar-system.org/ -- and I agree this PR is an improvement...

... even though in the example @aaruni96 shows it makes things worse in so far as the page that I'd be interested in might be this one which documents a method quo(R::MPolyRing, I::MPolyIdeal; ...) and it is listed at position 4 in the first screenshot, and not at all in the second screenshot. But I think the problem there is that even though MPolyRing occurs in the preview of the search results (albeit truncated with an ellipsis... ) this is not actually taken into account by search -- i.e. the first screen shot hits the desired result only "by accident". Which is bad by itself, but perhaps a separate issue?

@Hetarth02
Copy link
Contributor

A suggestion from my side would be, rather than us deciding the combine method we could do something where the user can choose the way search works. I think there were talks of something like integrating advance search functionalities which the user can toggle and tinker with if they want or just use vanilla search if they just want anything quickly.

@aaruni96
Copy link
Contributor Author

aaruni96 commented Jun 7, 2024

and not at all in the second screenshot

I did not notice this until you pointed it out, and yes, the result we want is not among any of the 35 results, and I don't understand why.

If you search on this PR for quo MPoly.ring, the result we want is the first result. If you search for quo.*Mpoly.ing, its the first result, and the match is even highlighted as a full match. But searching for quo MPolyRing or even quo.*MpolyRing excludes that result. For whatever reason, its only possibly to match the "R" from "MPolyRing" using . in regx.

Screenshot1: quo MPolyRing
image

Screenshot2: quo MPoly.ing
image

Screenshot3: quo.*Mpoly.ing
image

@aaruni96
Copy link
Contributor Author

aaruni96 commented Jun 7, 2024

I should add that the query quo.*MpolyRing in the currently deployed https://docs.oscar-system.org (without this PR) does not have this peculiar problem with "R" in "MPolyRing".

I don't know why the change introduced in this PR should make a difference in this regard

Screenshot:

image

@fingolfin
Copy link
Contributor

In Google you use to be to search for things like

"A B"

which means "literal match"

And in at least some search engines one used to be able to enter "AND", "OR", etc. in expressions. so you could search "A AND B" to have both occur. Of course that has its own drawbacks, but at least it is simple and quick to explain...

But I really wonder: how often do you really search for "things that mentioner A OR B" in a documentation? And if you want that, you can always first search for A and look at the results; then search for B and look at the results.

As such I think it makes a lot of sense to have "AND" on by default, and I am not convinced it is a useful user preference to allow allow specifying "OR" somehow -- though I am happy to be proven wrong. But if this was my project to run, I think I'd first switch to "AND" then wait for concrete user feedback/complaints before deciding how to handle "OR" support.

But it is not, so ... how shall we proceed? Some way to AND search seems crucial to me, Documenter's search is pretty bad right now in my experience / for my use cases, sadly (no offense intended -- I realize this is a very difficult thing to get right across so many diverse projects)

@Hetarth02
Copy link
Contributor

Hetarth02 commented Jun 12, 2024

Some way to AND search seems crucial to me

What we can do is invite some package maintainers to test the AND config and if everything goes well, we can merge this for now. While adding a way for the maintainers/users to fine-tune the search their way.

What are your thoughts on this?

CC @mortenpi @fingolfin

Copy link
Member

@mortenpi mortenpi left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

I also agree with @fingolfin in that I think AND makes more sense as the default behavior, and also that allowing users to toggle this behavior is not worth the complexity -- I think it would be very rare for anyone to want to do complex queries in on a documentation page.

@aaruni96
Copy link
Contributor Author

As a side note (and maybe this should be documented as its own issue) : how do you fix the formatting / run Prettier.js ? I am not really a webdev and not familiar with the tooling, and couldn't find the answer in Documenter's dev docs / contributing guidelines.

@Hetarth02
Copy link
Contributor

Hetarth02 commented Jun 13, 2024

@aaruni96 Most editors will have some way to install/enable prettier plugin. You can follow this guide, https://prettier.io/docs/en/install.

@fingolfin
Copy link
Contributor

Most editors will have some way to install/enable prettier plugin

Citation needed.

@fingolfin
Copy link
Contributor

Yay, all tests pass!

@mortenpi mortenpi merged commit a75c6e1 into JuliaDocs:master Jun 14, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants