NightOwl888 commented on code in PR #1:
URL: 
https://github.com/apache/lucenenet-codeanalysis-dev/pull/1#discussion_r1987886261


##########
src/Lucene.Net.CodeAnalysis.Dev/AnalyzerReleases.Unshipped.md:
##########
@@ -0,0 +1,9 @@
+### New Rules

Review Comment:
   It looks like you meant to use 
[Microsoft.CodeAnalysis.PublicApiAnalyzers](https://github.com/[dotnet/roslyn-analyzers](https://github.com/dotnet/roslyn-analyzers?tab=readme-ov-file#microsoftcodeanalysispublicapianalyzers)?tab=readme-ov-file#microsoftcodeanalysispublicapianalyzers),
 but I don't see it configured in the `.csproj` file. Shannon mentioned it on 
Slack here along with a link to the docs: 
https://the-asf.slack.com/archives/C03FFRQQ2RM/p1726239534319799 and it is 
added in the `Directory.Build.props` file of roslyn-analyzers 
[here](https://github.com/dotnet/roslyn-analyzers/blob/1e98fa3f107e780569a167a9250bb600778456dc/src/Directory.Build.props#L42).
 
   
   We should also copy over the [ReleaseTrackingAnalyzers.Help.md 
documentation](https://github.com/dotnet/roslyn-analyzers/blob/main/src/Microsoft.CodeAnalysis.Analyzers/ReleaseTrackingAnalyzers.Help.md),
 since it is not immediately obvious how to use these files.
   
   -----------------------------
   
   Also, it would be best to use the [procedure for accepting analyzers that 
Microsoft 
uses](https://github.com/dotnet/roslyn-analyzers/blob/main/GuidelinesForNewRules.md).
 The bits to copy over are in: 
https://github.com/search?q=repo%3Adotnet%2Froslyn-analyzers%20DiagnosticCategoryAndIdRanges.txt&type=code.
 Ideally, we would use the same set of categories (and perhaps add new ones) 
but have different analyzer code ranges.
   
   The code that validates the `DiagnosticCategoryAndIdRanges.txt` file is: 
https://github.com/dotnet/roslyn-analyzers/blob/345816fb41f40db5463ecc9b4308d29fbc0e4eaf/src/Microsoft.CodeAnalysis.Analyzers/Core/MetaAnalyzers/DiagnosticDescriptorCreationAnalyzer_IdRangeAndCategoryValidation.cs#L29.
   
   It looks like we can manage the project this way, but let me know if it is 
not sensible to do this.



##########
.github/workflows/Lucene.Net.CodeAnalysis.Dev.yml:
##########
@@ -0,0 +1,28 @@
+# This workflow will build a .NET project
+# For more information see: 
https://docs.github.com/en/actions/automating-builds-and-tests/building-and-testing-net
+
+name: Lucene.Net.CodeAnalysis.Dev
+
+on:
+  push:
+    branches: [ "main" ]
+  pull_request:
+    branches: [ "main" ]

Review Comment:
   Filtering on `main` means that this CI won't run unless the pull request is 
targeting the `main` branch. To make it possible to run on any branch (which is 
what normally happens when submitting a PR), we should remove the branch 
filter, as follows:
   
   ```
     pull_request:
   ```
   
   Also, it is usually useful to be able to run a workflow manually in the 
GitHub Actions control panel. For that, we should also use `workflow_dispatch`.
   
   ```
   on:
     workflow_dispatch:
     pull_request:
   ```
   
   We should change the
   
   ```
   on:
     push:
       branches: [ "main" ]
   ```
   
   because it will just re-run the CI after it is merged to `main` after it has 
already been run on the PR.
   
   Ideally, we won't push anything directly to `main` except for Git tags, but 
that should be the trigger for the deployment workflow, not the CI workflow. 
There are a couple of different options here:
   
   1. Use a release branching scheme, such as `release/v1.0.0`. This allows 
mainline development while the release is being done. This requires some setup 
with the `nbgv` .NET tool to ensure each branch has its own version. In this 
case, the release is done from the branch and then it is merged back to `main`.
   2. Release directly from `main`.
   
   Given that we already use the first approach in other projects (such as 
J2N), that would probably be easiest to use. So, we would need the trigger to 
only happen when we push a `release/v*` branch.



##########
src/Lucene.Net.CodeAnalysis.Dev/Lucene.Net.CodeAnalysis.Dev.csproj:
##########
@@ -3,16 +3,21 @@
   <PropertyGroup>
     <TargetFramework>netstandard2.0</TargetFramework>
     <IsPackable>false</IsPackable>
+    <Nullable>enable</Nullable>
+    <LangVersion>latest</LangVersion>
+    <EnforceExtendedAnalyzerRules>true</EnforceExtendedAnalyzerRules>
+    <IsRoslynComponent>true</IsRoslynComponent>
+    <TreatWarningsAsErrors>true</TreatWarningsAsErrors>
 
-    <Version>1.0.2</Version>
-    
-    <!-- Avoid ID conflicts with the package project. -->
-    <PackageId>*$(MSBuildProjectFile)*</PackageId>
+    <Version>1.0.0</Version>

Review Comment:
   Visual Studio requires `AssemblyVersion` to be bumped on every code change 
because of caching. Let's set up 
[Nerdbank.GitVersioning](https://www.nuget.org/packages/Nerdbank.GitVersioning/),
 which not only bumps the version number but ensures a commit will always 
produce the same version number.
   
   
https://dotnet.github.io/Nerdbank.GitVersioning/docs/build-systems/msbuild.html
   https://dotnet.github.io/Nerdbank.GitVersioning/docs/versionJson.html
   
https://[raw.githubusercontent.com/dotnet/Nerdbank.GitVersioning/main/src/NerdBank.GitVersioning/version.schema.json](https://raw.githubusercontent.com/dotnet/Nerdbank.GitVersioning/main/src/NerdBank.GitVersioning/version.schema.json)
   
   We should use:
   
   1. Don't configure "precision" for "assemblyVersion", since we want to use 
major.minor always. We will never have a patch. Developers who are building on 
branches other than `main` should be made aware that they will need to purge 
their cache to avoid `assemblyVersion` conflicts with `main`.
   2. Use the `main` branch as the release branch. Ideally, we would just push 
a git tag to `main` and the CI would detect when the top level commit is tagged 
and immediately push the package to NuGet.org. This will ensure that no release 
will happen without a git tag. Of course, when the top commit is not tagged, we 
just want CI to do a build and test only.
   3. Set `NBGV_EmitThisAssemblyClass` to `false`. We generally don't need the 
internal version properties.
   
   Note that Microsoft is using an internal project called 
[Arcade](https://github.com/dotnet/arcade/blob/main/Documentation/CorePackages/Versioning.md#recommended-settings)
 to bump the version number, as [they are setting `AutoGenerateAssemblyVersion` 
in the project 
file](https://github.com/dotnet/roslyn-analyzers/blob/main/Directory.Build.props#L7).
 NerdBank.GitVersioning is already on NuGet, though and has a .NET tool, nbgv 
to assist with cloud builds and release branching. The version is incremented 
for each commit automatically.
   



##########
src/Lucene.Net.CodeAnalysis.Dev/Resources.resx:
##########
@@ -138,7 +138,7 @@
     <comment>The format-able message the diagnostic displays.</comment>
   </data>
   <data name="LuceneDev1001_AnalyzerTitle" xml:space="preserve">
-    <value>Floating point types should be formatted with 
J2N.Numerics.Single.ToString() or J2N.Numerics.Double.ToString().</value>
+    <value>Floating point types should be formatted with J2N methods</value>

Review Comment:
   This isn't specific enough. We should call out 
J2N.Numerics.Single.ToString() or J2N.Numerics.Double.ToString().



##########
.github/workflows/Lucene.Net.CodeAnalysis.Dev.yml:
##########
@@ -0,0 +1,28 @@
+# This workflow will build a .NET project

Review Comment:
   I don't know if it is still an issue, but for the workflows in the 
`lucenenet` repo, we had to convert the file name to use `-` rather than `.` 
for all but the file extension because GitHub Actions didn't run the workflow 
when it was named like this. Also, this isn't named after the test project, 
which is what we did in the lucenenet repo. So, let's change the name to:
   
   `Lucene-Net-Tests-CodeAnalysis-Dev.yml`
   
   Ideally, we would have a separate workflow that triggers off of
   
   ```
   on:
     push:
       branches: [ "main" ]
   ```
   
   that also only runs when the top commit has a git tag that packs the `NuGet` 
and pushes it to `NuGet.org`. Perhaps that workflow could be named
   
   `Lucene-Net-Deploy-CodeAnalysis-Dev.yml`
   
   Then we can release simply by pushing a Git tag.
   



##########
src/Lucene.Net.CodeAnalysis.Dev.Sample/Lucene.Net.CodeAnalysis.Dev.Sample.csproj:
##########
@@ -0,0 +1,14 @@
+<Project Sdk="Microsoft.NET.Sdk">

Review Comment:
   Shouldn't the name of this project be `Lucene.Net.Sample.CodeAnalysis.Dev` 
to be consistent? Either that or we should rename the tests project to 
`Lucene.Net.CodeAnalysis.Dev.Tests`.



##########
src/Lucene.Net.CodeAnalysis.Dev.Sample/Lucene.Net.CodeAnalysis.Dev.Sample.csproj:
##########
@@ -0,0 +1,14 @@
+<Project Sdk="Microsoft.NET.Sdk">

Review Comment:
   For this type of thing to work in Visual Studio, the 
`Lucene.Net.CodeAnalysis.Dev` project would need to auto increment the assembly 
version number each time it is changed because it caches based on assembly 
version. NerdBank.GitVersioning will handle that bit, so that will be fine.
   
   Ideally, when running in Visual Studio, we would run the VSIX project which 
would open the *Visual Studio Test Instance* . This would allow us to open a 
sample project like this one, but having a `ProjectReference` back to 
`Lucene.Net.CodeAnalysis.Dev` will create a circular reference. We don't have 
to open this project, though. Usually, I just create a new project and do edits 
to test individual rules. It may be a bit confusing to have many different rule 
violations in the same project.
   
   Also, it seems like there is a lot of code duplication from what is in the 
strings in the tests. This is going to be difficult to keep in sync with the 
tests. But I guess if this is the only way to make it compatible with Ryder and 
other IDEs, it should be fine.



##########
src/Lucene.Net.CodeAnalysis.Dev/Resources.Designer.cs:
##########
@@ -60,135 +45,90 @@ internal Resources() {
             }
         }
         
-        /// <summary>
-        ///   Looks up a localized string similar to Floating point types 
should not be compared for exact equality..

Review Comment:
   These comments are very helpful when trying to understand the text that will 
be displayed for a given resource. Is there a way to make it generate these 
comments again?



##########
src/Lucene.Net.CodeAnalysis.Dev/Resources.resx:
##########
@@ -150,31 +150,31 @@
     <comment>The format-able message the diagnostic displays.</comment>
   </data>
   <data name="LuceneDev1002_AnalyzerTitle" xml:space="preserve">
-    <value>Floating point type arithmetic needs to be checked on x86 in .NET 
Framework.</value>
+    <value>Floating point type arithmetic needs to be checked</value>

Review Comment:
   This isn't specific enough. This problem is specific to x86 on .NET 
Framework with optimizations disabled. There is no other way to reproduce it 
AFAIK.



-- 
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

Reply via email to