Skip to content
This repository was archived by the owner on Oct 17, 2022. It is now read-only.

Conversation

@lbboe
Copy link
Contributor

@lbboe lbboe commented Jun 7, 2019

Overview

Add documentation about using search indexes to query a database using Lucene Query Parser Syntax.

GitHub issue number

#417

Copy link
Member

@wohali wohali 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 a great contribution for the search index documentation. Thank you for your work! I left a few small comments.

Unfortunately, the actual API reference for the new endpoint /_search_analyze is missing. (I skimmed, there may be more, but I think this is the only one.) The reference can remain, but it should be broken out and described with the proper syntax as a new server-level endpoint somewhere inside src/api/server/common.rst.

We overlooked doing this for Mango and it's still a problem (we don't properly document the /_explain and /_find endpoints.) Let's not make the same mistake with Search.

@kocolosk
Copy link
Member

I just added some documentation of the config settings for Dreyfus in the main PR that probably makes sense to include over here: apache/couchdb@0d32708

@kocolosk kocolosk requested review from flimzy and wohali June 27, 2019 01:18
Copy link
Member

@kocolosk kocolosk left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @lbboe !

Copy link
Member

@kocolosk kocolosk left a comment

Choose a reason for hiding this comment

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

Actually, found a couple of additional nits, almost there!

Copy link
Member

@kocolosk kocolosk left a comment

Choose a reason for hiding this comment

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

OK now with actual comments included 😄

Copy link
Member

@wohali wohali left a comment

Choose a reason for hiding this comment

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

Many of my comments from last time are still unaddressed. Please resolve.

@kocolosk
Copy link
Member

@wohali hmm I thought the Clouseau fallout and the analyzer list were resolved. You’re definitely right about the geo one, though.

@wohali
Copy link
Member

wohali commented Jun 27, 2019

@kocolosk mostly, it's the lines-too-long stuff. I think our automated checker isn't faulting the source on it right now, but invariably, it will start doing so as soon as it hits master ;)

@kocolosk
Copy link
Member

That’s weird. The linter was definitely faulting too-long-lines earlier in the PR, and Lora fixed everything it was complaining about. I’ll take a look

@kocolosk
Copy link
Member

make check runs clean locally, fwiw

Copy link
Member

@wohali wohali left a comment

Choose a reason for hiding this comment

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

Looking much better, one minor change and one content shuffle still required.

@kocolosk
Copy link
Member

OK, I think this is ready to merge, all requested changes have been made and I'm personally happy with the layout and organization of the User Guide and the additions to the API Reference. Thanks @lbboe for this contribution and thanks @flimzy and @wohali for the marathon review!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants