NightOwl888 commented on code in PR #961:
URL: https://github.com/apache/lucenenet/pull/961#discussion_r1770608515
##########
src/docs/LuceneDocsPlugins/LuceneDocsPlugins.csproj:
##########
@@ -39,32 +39,29 @@ under the License.
<PropertyGroup Label="Assembly Publishing">
<IsPublishable>true</IsPublishable>
</PropertyGroup>
-
+
<PropertyGroup Label="Assembly Signing">
<!-- Ensure this doesn't inherit the strong naming since this tool will no
work with a signed assembly due to it's references -->
<SignAssembly>false</SignAssembly>
</PropertyGroup>
-
- <ItemGroup>
- <Compile Remove="packages\**" />
- <EmbeddedResource Remove="packages\**" />
- <None Remove="packages\**" />
- </ItemGroup>
<ItemGroup>
- <None Include="app.config" />
+ <PackageReference Include="Docfx.Plugins" Version="2.76.0" />
+ <PackageReference Include="Docfx.Common" Version="2.76.0" />
+ <PackageReference Include="System.Composition" Version="8.0.0" />
</ItemGroup>
<ItemGroup>
- <PackageReference Include="Microsoft.Composition" Version="1.0.31" />
- <PackageReference Include="Microsoft.DocAsCode.Dfm" Version="2.58.0" />
- <PackageReference Include="YamlDotNet" Version="6.0.0" />
- </ItemGroup>
-
- <ItemGroup>
- <Reference Include="Microsoft.CSharp" />
- <Reference Include="System" />
- <Reference Include="System.Web" />
+ <Compile Remove="EnvironmentVariableInlineRule.cs" />
Review Comment:
Looks like these files were deleted? If so, we don't need these references.
##########
websites/apidocs/docs.ps1:
##########
@@ -34,7 +34,8 @@ param (
[Parameter(Mandatory = $false)]
[int] $StagingPort = 8080
)
-$MinimumSdkVersion = "3.1.100" # Minimum Required .NET SDK (must not be a
pre-release)
+$MinimumSdkVersion = "8.0.204" # Minimum Required .NET SDK (must not be a
pre-release)
Review Comment:
We definitely don't want the minimum to be the latest version. There is no
reason why a machine with 8.0.100 installed shouldn't be able to run this. This
should technically be kept in sync with the version in the `runbuild.ps1` file.
##########
src/dotnet/tools/lucene-cli/docs/index.md:
##########
@@ -44,3 +44,18 @@ CLI command structure consists of the driver ("lucene"), the
command, and possib
lucene index check C:\my-index --verbose
lucene index fix C:\my-index
```
+
Review Comment:
The Powershell script kept adding these lines at the bottom when it was run,
but I usually just revert them before committing. Not sure if the plugin is
adding them also, but ideally we wouldn't change anything but the version
number.
There are a few other files with these extra lines, also.
##########
websites/site/site.ps1:
##########
@@ -43,21 +45,27 @@ if ($Clean) {
Remove-Item (Join-Path -Path $ToolsFolder "\*") -recurse -force
-ErrorAction SilentlyContinue
}
-New-Item "$ToolsFolder\tmp" -type directory -force
+# install docfx tool
+$InstallDocFx = $false
+try {
+ $InstalledDocFxVersion = (& docfx --version).Trim().Split('+')[0]
-# Go get docfx.exe if we don't have it
-New-Item "$ToolsFolder\docfx" -type directory -force
-$DocFxExe = "$ToolsFolder\docfx\docfx.exe"
-if (-not (test-path $DocFxExe))
-{
- Write-Host "Retrieving docfx..."
- $DocFxZip = "$ToolsFolder\tmp\docfx.zip"
- Invoke-WebRequest
"https://github.com/dotnet/docfx/releases/download/v2.58/docfx.zip"; -OutFile
$DocFxZip -TimeoutSec 60
- #unzip
- Expand-Archive $DocFxZip -DestinationPath (Join-Path -Path $ToolsFolder
-ChildPath "docfx")
+ if ([version]$InstalledDocFxVersion -lt [version]$DocFxVersion) {
+ Write-Host "DocFx is installed, but the version is less than
$DocFxVersion, will install it."
+ $InstallDocFx = $true
+ }
+ else {
+ Write-Host "DocFx is installed and the version is
$InstalledDocFxVersion."
+ }
+} catch {
+ Write-Host "DocFx is not installed or not in the PATH, will install it."
+ $InstallDocFx = $true
}
- Remove-Item -Recurse -Force "$ToolsFolder\tmp"
+if ($InstallDocFx -eq $true) {
+ Write-Host "Installing docfx global tool..."
+ dotnet tool install -g docfx --version $DocFxVersion
Review Comment:
This should work when developers are working on the site. But, I have never
tried installing as a global tool in CI so I am not sure what the implications
are. Could lack of permissions be a problem?
That said, it seems like detecting the environment is either GitHub Actions
or Azure DevOps and always doing a local install (with a specific version)
would probably be a more robust approach in CI.
##########
src/Lucene.Net.Highlighter/Highlight/WeightedSpanTermExtractor.cs:
##########
@@ -58,7 +58,7 @@ public WeightedSpanTermExtractor(string defaultField)
}
/// <summary>
- /// Fills a <see cref="T:IDictionary{string, WeightedSpanTerm}"/> with
<see cref="WeightedSpanTerm"/>s using the terms from the supplied <paramref
name="query"/>.
+ /// Fills a <see cref="T:IDictionary{string,WeightedSpanTerm}"/> with
<see cref="WeightedSpanTerm"/>s using the terms from the supplied <paramref
name="query"/>.
Review Comment:
I know this is probably quicker, but it seems like our project would be
easier to maintain if docfx were somehow setup to parse these even if they have
spaces in them. It would be a shame to have to remember to do this every time
we add documentation.
##########
src/dotnet/tools/lucene-cli/docs/index.md:
##########
@@ -11,7 +11,7 @@ The Lucene.NET command line interface (CLI) is a new
cross-platform toolchain wi
Perform a one-time install of the lucene-cli tool using the following dotnet
CLI command:
```console
-dotnet tool install lucene-cli -g --version 4.8.0-beta00016
+dotnet tool install lucene-cli -g --version EnvVar:LuceneNetVersion
Review Comment:
Okay, this is an issue that I discussed with Shannon previously. These docs
sit in our repo and GitHub has a designer that formats these. Users ***will***
see these markdown docs when browsing our repo. But having these weird tokens
in the docs instead of an actual functional command would be quite frustrating
for users (in fact, we had a complaint about it before).
So, can we fix this to detect a real version number and replace it like the
Powershell script did?
Another lesser issue is figuring out how to automate getting these version
numbers incremented in our markdown pages when we do a release. Or at least
documenting where they are and adding the steps to the [release
procedure](https://lucenenet.apache.org/contributing/make-release.html) to
manually update them. We would need to create a release branching scheme (on my
todo list) to ensure that the docs in the master branch aren't actually
updated until the release is completed.
##########
websites/apidocs/docfx.json:
##########
@@ -16,7 +16,7 @@
],
"dest": "obj/docfx/api/Lucene.Net",
"properties": {
- "TargetFramework": "netstandard2.0"
+ "TargetFramework": "net6.0"
Review Comment:
Is there a way to read these target frameworks from a common location? It
will be a lot of maintenance to change these every time a new target is added.
It's not critical to fix for this PR, but something to think about.
##########
websites/site/site.ps1:
##########
@@ -43,21 +45,27 @@ if ($Clean) {
Remove-Item (Join-Path -Path $ToolsFolder "\*") -recurse -force
-ErrorAction SilentlyContinue
}
-New-Item "$ToolsFolder\tmp" -type directory -force
+# install docfx tool
+$InstallDocFx = $false
+try {
+ $InstalledDocFxVersion = (& docfx --version).Trim().Split('+')[0]
-# Go get docfx.exe if we don't have it
-New-Item "$ToolsFolder\docfx" -type directory -force
-$DocFxExe = "$ToolsFolder\docfx\docfx.exe"
-if (-not (test-path $DocFxExe))
-{
- Write-Host "Retrieving docfx..."
- $DocFxZip = "$ToolsFolder\tmp\docfx.zip"
- Invoke-WebRequest
"https://github.com/dotnet/docfx/releases/download/v2.58/docfx.zip"; -OutFile
$DocFxZip -TimeoutSec 60
- #unzip
- Expand-Archive $DocFxZip -DestinationPath (Join-Path -Path $ToolsFolder
-ChildPath "docfx")
+ if ([version]$InstalledDocFxVersion -lt [version]$DocFxVersion) {
Review Comment:
Seems like we should pin to a specific DocFx version so we are guaranteed
repeatable runs and avoid different results on different machines, etc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
