-
Notifications
You must be signed in to change notification settings - Fork 639
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
Fixes #(959): Fix TermStats.TermText access, add CLI comments and 1 CLI bug fix #963
Fixes #(959): Fix TermStats.TermText access, add CLI comments and 1 CLI bug fix #963
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR.
A few things that could be changed on all/most of these:
- It would be helpful would be to drop a link to the exact command in the documentation. While I was going to suggest
/absoluteLatest
here, it would be best if we linked these comments to the same version of the app that they occur in. Especially for version-sensitive commands such asindex fix
orindex merge
, which require the same version oflucene-cli
to be installed as the version of Lucene.NET they are using. This is one of the downsides of not being able to run these commands on the software directly - it is up to the users to get the version right. If we had a single Powershell script that is run as part of the release procedure, we could update these versioned URLs and keep them in sync with the version that the user has installed (or if on the website, the version that they are currently viewing). Note that we could hold off on this and do it in a later PR. - Many of these classes are for direct use, as they have public static methods that can be called when referenced as a DLL. Technically, there is nothing stopping someone from creating a custom wrapper console app that cascades the call to
Main(object[])
. Instead of focusing on what is not intended, perhaps we should just focus on the fact that the main method is intended to be called throughlucene-cli
and usually the page for that command has links and/or documentation that explains how to use the command. - Some of the commands had no documentation that I could find, so they do little more than explain how to run the command (not what it is used for). At the time
lucene-cli
was documented, LLMs didn't exist. Perhaps we can provide more info if one or more of them is able to provide info. - Change the wording from
we provide a <see href="https://www.nuget.org/packages/lucene-cli">lucene-cli</see> with a command
towe provide a <a href="https://learn.microsoft.com/en-us/dotnet/core/tools/global-tools">.NET tool</a>, <see href="https://www.nuget.org/packages/lucene-cli"\>lucene-cli</see\>, with a command
.
The demos are a special case. See my inline comment for those.
...-cli/commands/benchmark/benchmark-find-quality-queries/BenchmarkFindQualityQueriesCommand.cs
Show resolved
Hide resolved
Hi Shad, In my view, the PR as it stands makes a positive contribution and moves the project forward. Rather than focusing on further refinement of the comments, I would suggest that the primary question should be whether this contribution, overall, improves the project. If it does, I believe it warrants approval. Thanks again for your consideration! |
Shoot, I did a bad merge. Going to force push to revert that |
…ed for command line CLI usage.
… to QualityQueriesFinder.Main(args)
bcecb4a
to
1899e51
Compare
Addressed the following:
Item 1 above and improving the documentation of the commands further can be a separate, future enhancement. |
Summary of the changes (Less than 80 chars)
Fixes #959: Fix TermStats.TermText access, add CLI comments and 1 CLI bug fix
Description