You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lucenenet.apache.org by "paulirwin (via GitHub)" <gi...@apache.org> on 2025/03/08 04:55:43 UTC
[PR] Update project for initial release [lucenenet-codeanalysis-dev]
paulirwin opened a new pull request, #1:
URL: https://github.com/apache/lucenenet-codeanalysis-dev/pull/1
This PR updates the project in preparation for our initial release under the Apache GitHub organization. It includes:
- Updates NuGet packages and target framework for unit test project
- Adds Sample project to test and debug analyzers
- Adds a launchSettings.json file to allow for debugging the analyzers, and removes VSIX support
- Fixes a failing unit test
- Fixes Roslyn meta-analyzer warnings (as errors)
- Adds GitHub Actions file
- Removes unused CodeFixes project (these will go into the same analyzers project)
--
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-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update project for initial release [lucenenet-codeanalysis-dev]
Posted by "paulirwin (via GitHub)" <gi...@apache.org>.
paulirwin commented on PR #1:
URL: https://github.com/apache/lucenenet-codeanalysis-dev/pull/1#issuecomment-2727726749
@NightOwl888 I split out creating rulesets as https://github.com/apache/lucenenet/issues/1143
Ready for re-review.
--
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-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update project for initial release [lucenenet-codeanalysis-dev]
Posted by "NightOwl888 (via GitHub)" <gi...@apache.org>.
NightOwl888 commented on code in PR #1:
URL: https://github.com/apache/lucenenet-codeanalysis-dev/pull/1#discussion_r1992796684
##########
src/Lucene.Net.CodeAnalysis.Dev/AnalyzerReleases.Unshipped.md:
##########
@@ -0,0 +1,9 @@
+### New Rules
Review Comment:
The fact that there are currelty only 5 analyzers is a symptom that this hasn't been easy enough to manage in the past.
- There are many project maintenance tasks that would ideally be made into Roslyn analyzers than what are currently open. For example, [TestApiConsistency](https://github.com/apache/lucenenet/blob/a96e768d14431cf9d85e394a333cd10c9112e975/src/Lucene.Net.TestFramework/Support/ApiScanTestBase.cs) (and all of its support attributes) can be made into code analyzers.
- There were search analyzers that I ended up deleting because there was no logical way to categorize them so they wouldn't impede development.
- There are several open issues regarding analyzers, some of which will probably end up with at least half a dozen different IDs.
My point is, I can see this easily reaching 50 or more analyzers before the Lucene.Net release. We will also have analyzers in ICU4N and it would be ideal to use the same process to manage ID uniqueness. Since Microsoft has been doing this for a while, it seems like the best idea is to follow their lead rather than go off the map and try something that may or may not work for the long haul.
--
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-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update project for initial release [lucenenet-codeanalysis-dev]
Posted by "paulirwin (via GitHub)" <gi...@apache.org>.
paulirwin commented on code in PR #1:
URL: https://github.com/apache/lucenenet-codeanalysis-dev/pull/1#discussion_r1992678142
##########
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:
After posting that last comment, it gave me the idea to go looking in some other places, and I found that VS will use the analyzer title when generating suppression messages or editor config comments. So, some usage there, but not as commonly used in the UI or MSBuild messages as the message format is.
--
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-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update project for initial release [lucenenet-codeanalysis-dev]
Posted by "NightOwl888 (via GitHub)" <gi...@apache.org>.
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-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update project for initial release [lucenenet-codeanalysis-dev]
Posted by "paulirwin (via GitHub)" <gi...@apache.org>.
paulirwin commented on code in PR #1:
URL: https://github.com/apache/lucenenet-codeanalysis-dev/pull/1#discussion_r1992662677
##########
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:
I expanded on this a little without adding a period in the title. FWIW, I can't tell where the title is actually used... it doesn't appear when there's an actual diagnostic raised. But, added more detail anyways.
--
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-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update project for initial release [lucenenet-codeanalysis-dev]
Posted by "paulirwin (via GitHub)" <gi...@apache.org>.
paulirwin commented on code in PR #1:
URL: https://github.com/apache/lucenenet-codeanalysis-dev/pull/1#discussion_r1992700796
##########
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:
Split out adding Nerdbank.GitVersioning as separate issue https://github.com/apache/lucenenet/issues/1142
--
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-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update project for initial release [lucenenet-codeanalysis-dev]
Posted by "paulirwin (via GitHub)" <gi...@apache.org>.
paulirwin commented on code in PR #1:
URL: https://github.com/apache/lucenenet-codeanalysis-dev/pull/1#discussion_r1992676800
##########
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:
Added "on some x86 platforms" to add more detail back without adding a period to the title.
--
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-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update project for initial release [lucenenet-codeanalysis-dev]
Posted by "paulirwin (via GitHub)" <gi...@apache.org>.
paulirwin commented on code in PR #1:
URL: https://github.com/apache/lucenenet-codeanalysis-dev/pull/1#discussion_r1993656488
##########
src/Lucene.Net.CodeAnalysis.Dev/AnalyzerReleases.Unshipped.md:
##########
@@ -0,0 +1,9 @@
+### New Rules
Review Comment:
First, it's worth pointing out that there are two possible problems being conflated here:
1. Two (or more) contributors working in parallel and needing to reserve IDs so they are unique, and don't need to rework their PRs
2. Ensuring that IDs in the project are unique within the working directory of the project at build time, and that they match the categories they're expected to be in
These are not the same problems. Microsoft's solution only solves the latter; it does not appear like they are solving the former with this.
If you look at [the history of that DiagnosticCategoryAndIdRanges.txt file](https://github.com/dotnet/roslyn-analyzers/commits/main/src/Utilities/Compiler/DiagnosticCategoryAndIdRanges.txt), you can see that it is only incremented as part of the PRs. That means that it alone does not solve the ID uniqueness/reservation problem once it is integrated, or having to avoid PR rework. If another contributor is working on one in parallel, this file (how they use it) does not provide global uniqueness guarantees, because it is only incremented in main when it is merged (de facto, anyways, based on historical evidence). The contributor might have to update their PR to change their IDs if another person was working on the same ID. Their process just ensures the file is correct and the IDs are unique after already being integrated; it doesn't do anything to help the "stepping on each others' toes" problem before integration. Both are valid problems, it should be noted.
My proposed solution does solve the former problem of reservation (as best as is possible in git), with a simpler solution. And by solving the former problem, it solves a Pareto-optimal amount of the second problem, because if you're reserving IDs in advance of developing the analyzers, then by definition (assuming that reservation was done correctly) they would be unique within the working directory of the project (either in your PR feature branch, or integrated into main) too.
We would use the same file/format, but we would treat what is in main as the source of truth. You would reserve an ID (or multiple) in advance by committing a change to that file to bump the ID. (If during development you discover you need more IDs, no problem, just bump the file again in main.) This way, you would get a merge conflict if you're trying to bump an older version of main, and would have to bump it again to a new ID. Then, with your ID(s) in hand, you go and submit the PR, which should be conflict-free ID-wise from any other PRs going on at the same time (that have also previously reserved theirs in main).
Also, something I left out from my prior message. If someone is not a committer to the repo, they can just request in the issue for another committer to reserve one or more IDs for them, then someone like me would be happy to do that, commit the change to main, and let the contributor know which IDs were reserved to them. No problem.
If we do not do this, then we still have the first problem, even if we write a great meta-analyzer. We can add one later (I think we should split that out as a separate issue), and we can manually ensure compliance in PR review in the short term. The meta-analyzer will be some good process-hardening that will be nice for the long-term success of this project but is not needed right now (I'd suggest we do it between beta 18 and GA to not hold up the beta 18 release over this). Again, I'm not saying to not use the file, quite the opposite as noted above. Just that we don't need the meta-analyzer at this time. This would be a simple solution that starts us on the path to that, solves most of the big uniqueness problems in advance, while letting this get hardened in an automated way later.
--
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-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update project for initial release [lucenenet-codeanalysis-dev]
Posted by "paulirwin (via GitHub)" <gi...@apache.org>.
paulirwin commented on code in PR #1:
URL: https://github.com/apache/lucenenet-codeanalysis-dev/pull/1#discussion_r1992692641
##########
.github/workflows/Lucene.Net.CodeAnalysis.Dev.yml:
##########
@@ -0,0 +1,28 @@
+# This workflow will build a .NET project
Review Comment:
Deployment can come in a later PR. Renamed workflow file to match renamed unit test project name.
--
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-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update project for initial release [lucenenet-codeanalysis-dev]
Posted by "NightOwl888 (via GitHub)" <gi...@apache.org>.
NightOwl888 commented on code in PR #1:
URL: https://github.com/apache/lucenenet-codeanalysis-dev/pull/1#discussion_r1993753955
##########
src/Lucene.Net.CodeAnalysis.Dev/AnalyzerReleases.Unshipped.md:
##########
@@ -0,0 +1,9 @@
+### New Rules
Review Comment:
Gotcha. Pre-reserving the IDs on main sounds like it should work, provided there is documentation for the procedure (both how to submit and accept PRs). The submission part could just be put into the template, but for PMC who only touch this repo only occasionally, there should be a document for the exact steps on how to approve an issue, assign IDs, and accept a PR.
Should we add a label to indicate whether the ID(s) for a particular issue are assigned (in addition to an `approved-rule` label)?
--
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-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update project for initial release [lucenenet-codeanalysis-dev]
Posted by "paulirwin (via GitHub)" <gi...@apache.org>.
paulirwin commented on code in PR #1:
URL: https://github.com/apache/lucenenet-codeanalysis-dev/pull/1#discussion_r1992691024
##########
.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:
We can sort out release/versioning/etc. concerns in a later PR. For now, I've made those changes for the pull request and dispatch triggers. On most other projects, I like doing a main push build as well, since sometimes there might be subtle differences when merged/squashed that can cause build or test failures... but I'm also fine with leaving that off for now.
--
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-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update project for initial release [lucenenet-codeanalysis-dev]
Posted by "NightOwl888 (via GitHub)" <gi...@apache.org>.
NightOwl888 commented on PR #1:
URL: https://github.com/apache/lucenenet-codeanalysis-dev/pull/1#issuecomment-2719822940
One thing I forgot about in the review was that since this project will be a private dependency of all of the Lucene.Net projects, it should be strong named to avoid the build warning. Ideally, we would use the same SNK from the lucenenet repo, but I see no issue with copying it here, since it is unlikely we will ever change it.
--
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-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update project for initial release [lucenenet-codeanalysis-dev]
Posted by "paulirwin (via GitHub)" <gi...@apache.org>.
paulirwin commented on code in PR #1:
URL: https://github.com/apache/lucenenet-codeanalysis-dev/pull/1#discussion_r1992637120
##########
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:
Turns out this is a known issue in Rider. https://youtrack.jetbrains.com/issue/RIDER-74074/PublicResXFileCodeGenerator-does-not-generate-comments-and-the-order-of-properties-changes
I worked around this by using the MSBuild:Compile generator on the resx, which generates the .Designer.cs file into the obj folder, so it can be removed from source control. When done this way, the generated file has comments, and we get the benefit of not having to merge this file.
--
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-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update project for initial release [lucenenet-codeanalysis-dev]
Posted by "NightOwl888 (via GitHub)" <gi...@apache.org>.
NightOwl888 merged PR #1:
URL: https://github.com/apache/lucenenet-codeanalysis-dev/pull/1
--
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-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update project for initial release [lucenenet-codeanalysis-dev]
Posted by "paulirwin (via GitHub)" <gi...@apache.org>.
paulirwin commented on code in PR #1:
URL: https://github.com/apache/lucenenet-codeanalysis-dev/pull/1#discussion_r1997776222
##########
src/Lucene.Net.CodeAnalysis.Dev/AnalyzerReleases.Unshipped.md:
##########
@@ -0,0 +1,9 @@
+### New Rules
Review Comment:
Readme and IDs file pushed up; let me know what you think. Feel free to add comments to those files as needed.
--
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-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update project for initial release [lucenenet-codeanalysis-dev]
Posted by "paulirwin (via GitHub)" <gi...@apache.org>.
paulirwin commented on code in PR #1:
URL: https://github.com/apache/lucenenet-codeanalysis-dev/pull/1#discussion_r1990006541
##########
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:
The Microsoft.CodeAnalysis.Analyzers package does not allow for having periods in analyzer titles, which is why this was removed. If you have a suggestion for how to reference those without periods, let me know.
--
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-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update project for initial release [lucenenet-codeanalysis-dev]
Posted by "paulirwin (via GitHub)" <gi...@apache.org>.
paulirwin commented on code in PR #1:
URL: https://github.com/apache/lucenenet-codeanalysis-dev/pull/1#discussion_r1990007448
##########
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:
Also, the idea is that you would be looking at the formatted description of the message for what the details are, not the analyzer title.
--
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-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update project for initial release [lucenenet-codeanalysis-dev]
Posted by "paulirwin (via GitHub)" <gi...@apache.org>.
paulirwin commented on code in PR #1:
URL: https://github.com/apache/lucenenet-codeanalysis-dev/pull/1#discussion_r1993786025
##########
src/Lucene.Net.CodeAnalysis.Dev/AnalyzerReleases.Unshipped.md:
##########
@@ -0,0 +1,9 @@
+### New Rules
Review Comment:
I'm open to that label, although we could make that part of the definition of `approved-rule` as well. No strong preference there.
I'll begin drafting changes to the readme to account for this process.
--
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-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update project for initial release [lucenenet-codeanalysis-dev]
Posted by "paulirwin (via GitHub)" <gi...@apache.org>.
paulirwin commented on code in PR #1:
URL: https://github.com/apache/lucenenet-codeanalysis-dev/pull/1#discussion_r1992647119
##########
src/Lucene.Net.CodeAnalysis.Dev.Sample/Lucene.Net.CodeAnalysis.Dev.Sample.csproj:
##########
@@ -0,0 +1,14 @@
+<Project Sdk="Microsoft.NET.Sdk">
Review Comment:
Renamed unit test project, other comments addressed above.
--
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-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update project for initial release [lucenenet-codeanalysis-dev]
Posted by "paulirwin (via GitHub)" <gi...@apache.org>.
paulirwin commented on code in PR #1:
URL: https://github.com/apache/lucenenet-codeanalysis-dev/pull/1#discussion_r1990018994
##########
src/Lucene.Net.CodeAnalysis.Dev.Sample/Lucene.Net.CodeAnalysis.Dev.Sample.csproj:
##########
@@ -0,0 +1,14 @@
+<Project Sdk="Microsoft.NET.Sdk">
Review Comment:
I'd prefer renaming the tests project if you're okay with that. It seems like "Lucene.Net.CodeAnalysis.Dev" naturally feels like the prefix of the projects, via the solution name.
re: duplciated code, the Sample project is for a playground to experiment with the analyzers or debug them locally, not to assert i.e. in CI builds that they work correctly. I intentionally just copied the test examples into the Sample project just as a starting point; they might diverge in the future.
re: Visual Studio needing a new version number, I don't believe this is true anymore. I was able to make a change in Visual Studio 2022 (and Rider, fwiw) and after building it was immediately reflected in the sample project. I think Microsoft might have fixed this in an updated Analyzer SDK. It'd be great if you could give it a try yourself to confirm. That doesn't invalidate the need for gitversioning, but just an FYI on that.
--
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-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update project for initial release [lucenenet-codeanalysis-dev]
Posted by "paulirwin (via GitHub)" <gi...@apache.org>.
paulirwin commented on code in PR #1:
URL: https://github.com/apache/lucenenet-codeanalysis-dev/pull/1#discussion_r1990004571
##########
src/Lucene.Net.CodeAnalysis.Dev/AnalyzerReleases.Unshipped.md:
##########
@@ -0,0 +1,9 @@
+### New Rules
Review Comment:
I'll work on the rest, but in regard to "It looks like you meant to use [Microsoft.CodeAnalysis.PublicApiAnalyzers](https://github.com/%5Bdotnet/roslyn-analyzers%5D(https://github.com/dotnet/roslyn-analyzers?tab=readme-ov-file#microsoftcodeanalysispublicapianalyzers)?tab=readme-ov-file#microsoftcodeanalysispublicapianalyzers)" - nope, Microsoft.CodeAnalysis.Analyzers is a different package that provides meta-analyzers that make sure you're creating analyzers properly. That is a different package than PublicApiAnalyzers, which is for analyzing changes to your public APIs. Microsoft.CodeAnalysis.Analyzers reads in these Markdown files and expects them to be in the specific format per the docs that you linked to.
I do not think we need Microsoft.CodeAnalysis.PublicApiAnalyzers as this is not a public API per se like Roslyn analyzers are (per your example link). That could be useful on the main lucenenet project (including our public analyzers), though.
Re: the docs, I think we should just link to those docs in the README, not copy the docs here. That way if they improve the docs or fix documentation bugs there, we don't have to keep our version in sync.
--
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-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update project for initial release [lucenenet-codeanalysis-dev]
Posted by "paulirwin (via GitHub)" <gi...@apache.org>.
paulirwin commented on code in PR #1:
URL: https://github.com/apache/lucenenet-codeanalysis-dev/pull/1#discussion_r1990006943
##########
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:
Same as above, periods not allowed. If you have a suggestion, let me know.
--
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-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update project for initial release [lucenenet-codeanalysis-dev]
Posted by "paulirwin (via GitHub)" <gi...@apache.org>.
paulirwin commented on code in PR #1:
URL: https://github.com/apache/lucenenet-codeanalysis-dev/pull/1#discussion_r1992727258
##########
src/Lucene.Net.CodeAnalysis.Dev/AnalyzerReleases.Unshipped.md:
##########
@@ -0,0 +1,9 @@
+### New Rules
Review Comment:
After removing the generated Resources.Designer.cs file (and having it auto-generated on build), having to merge/rebase to change rule IDs is no longer as big of an issue. After thinking about it more, I also don't think we're going to be having so many analyzers that this is a huge problem.
I'd like to propose we do a simpler solution: we can still have the text file of reserved IDs, but we treat main as the source of truth for this file, and allow committers to reserve an ID by committing a change to this file without having to go through a PR. They can then create the PR using that ID.
If another contributor comes along to add another analyzer, they will follow the process (which will be documented in the README/CONTRIBUTING file), pull latest main, add the next ID to the end, commit their change to the text file to "lock in" their ID, and then do their PR.
And if a contributor skips that part of the process and submits a PR for a conflicting ID, then it will be on them to reserve a new ID and update their PR with the new ID.
I don't think we need the complexity of having a test/meta-analyzer for this, it feels like overkill. We are currently at 5-ish analyzers, and I don't see us growing to where this will be a huge problem. We can also always add it later, too, if really needed.
--
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-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org