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