Fixes #(959): Fix TermStats.TermText access, add CLI comments and 1 CLI bug fix #963
Conversation
…ed for command line CLI usage.
… to QualityQueriesFinder.Main(args)
There was a problem hiding this comment.
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
/absoluteLatesthere, 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 fixorindex merge, which require the same version oflucene-clito 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-cliand 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-cliwas 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 commandtowe 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.
| /// method within a DLL can't be directly called from the command line so we | ||
| /// provide a <see href="https://www.nuget.org/packages/lucene-cli">lucene-cli</see> | ||
| /// with a command that maps to that method: analysis stempel-compile-stems. | ||
| /// </para> |
There was a problem hiding this comment.
I know this is the correct way to use an HTML paragraph tag, but I have found that using a single closed tag between paragraphs also works (<para/>) and replaces the <p> tag they use in Java without the need of an additional closing tag. Since that is the way it is used in most of the other code comments, could you please change them in this PR, as well?
There was a problem hiding this comment.
Yah, I went back and forth about that. but I like the <para>text </para> approach better in cases where a chunk of the text is luceneNET specific.
| @@ -25,7 +26,7 @@ public class Configuration : ConfigurationBase | |||
| { | |||
| public Configuration(CommandLineOptions options) | |||
| { | |||
| this.Main = (args) => QueryDriver.Main(args); | |||
| this.Main = (args) => QualityQueriesFinder.Main(args); | |||
| @@ -20,6 +20,13 @@ namespace Lucene.Net.Analysis.Ja.Util | |||
| * limitations under the License. | |||
| */ | |||
|
|
|||
| /// <summary> | |||
| /// LUCENENET specific: This class is not for direct use. In the Java implementation | |||
There was a problem hiding this comment.
Technically, the DictionaryBuilder.Build() method can be used from a calling app by calling into the DLL.
And it looks like all of the dependent types are public, so this could be used as a console app by copy and paste if one were to also bring in all of the dependencies.
Maybe just delete this doc comment. There is a lot that could go here to explain how to use this, but I see that the blog post I used to test it is listed in our docs for this command: https://lucenenet.apache.org/docs/4.8.0-beta00016/cli/analysis/kuromoji-build-dictionary.html#description.
I also looked up the procedure on ChatGPT, if you are interested: https://chatgpt.com/share/66f50a60-8e38-8005-9c29-f7633024787c
Maybe we could explain here or on the command below that the instructions on how to use this are on that page, since if a user starts here, they won't see the usage instructions.
|
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! |
Summary of the changes (Less than 80 chars)
Fixes #959: Fix TermStats.TermText access, add CLI comments and 1 CLI bug fix
Description