Increase severity of ConvertToPrimaryConstructor

[verdie-g]

The Github build consider ConvertToPrimaryConstructor as an error

Warning: src\JsonApiDotNetCore.OpenApi.Client\ApiResponse.cs:13 ConvertToPrimaryConstructor: Convert into primary constructor

when in Rider it's only shown as a hint.

I think the severity can be changed in the .editorconfig?

I think this affects many other hints but I only have this example for now, and it greatly affects the developer experience.

Also I'm not sure why the cleanup.ps1 didn't warn me about that 🤔

[bkoelman]

This is intentional. It's all explained at https://github.com/json-api-dotnet/JsonApiDotNetCore/blob/master/.github/CONTRIBUTING.md#pull-requests.

[verdie-g]

Coding style is validated during PR build, where we inject an extra settings layer that promotes various suggestions to warning level. This ensures a high-quality codebase without interfering too much when editing code

I don't understand why these warnings would not be enabled during development.

[verdie-g]

without interfering too much when editing code

A warning doesn't interfere code edition but having a build failing in the CI even after manually building, running tests, and running the custom cleaning script is kind of frustrating.

[bkoelman]

Coding style is validated during PR build, where we inject an extra settings layer that promotes various suggestions to warning level. This ensures a high-quality codebase without interfering too much when editing code

I don't understand why these warnings would not be enabled during development.

Because the squiggles are distracting while writing code. Warnings are supposed to alert you to potential mistakes. When a file has numerous squiggles all the time, they become background noise that gets ignored. I've worked a while with StyleCop, very annoying. I don't want to be bothered with whitespace violations, unused private methods, unreachable code, or when to use var when focussing on the logic itself. But I do want to be notified when dereferencing something that may be null. The rulesets are carefully tweaked for that. The cleanup usually takes care of things afterwards, when I'm ready for it. In VS, simply press Ctrl+E, F, and save the file. The cleanupcode.ps1 command-line tool is available for anyone without a Rider/Resharper license to auto-fix whitespace.

A warning doesn't interfere code edition but having a build failing in the CI even after manually building, running tests, and running the custom cleaning script is kind of frustrating.

You're just not using the right tools. Everything can be done locally. In VS, they show up under Extensions > Resharper > Inspect > Code Issues in Solution as suggestions. Rider has a similar dialog. For most of them, a refactoring is offered to resolve. For people without a license, the command-line tool inspectcode.ps1 is available.

[verdie-g]

What I'm not sure to understand is why the cleanupcode.ps1 doesn't have the same behavior when running locally or in the github workflow, i.e. why does the Warning: src\JsonApiDotNetCore.OpenApi.Client\ApiResponse.cs:13 ConvertToPrimaryConstructor: Convert into primary constructor doesn't appear locally?

[bkoelman]

That sounds like a warning from inspectcode, not cleanupcode.

[verdie-g]

Shouldn't cleanupcode take care of applying a fix?

I don't know what's the best solution but my point is that I find the current system is unfriendly to external contributors and I think it should give clearer hints when the code will not build on the CI.

[bkoelman]

Shouldn't cleanupcode take care of applying a fix?

You could ask JetBrains for that, but I suspect the answer is "no". cleanupcode optimizes whitespace, line breaks, etc. It's unaware of the semantic model, which is why you can run it on a subset of files. Because it doesn't understand your code above the syntax level, it performs no refactorings that potentially change behavior. That's why they built inspectcode: to verify configured rules and alert you. Then you can choose how to resolve by selecting one of their built-in refactorings, solve it manually in a different way, or suppress because it doesn't apply.

I don't know what's the best solution but my point is that I find the current system is unfriendly to external contributors and I think it should give clearer hints when the code will not build on the CI.

We've had other community members use these tools in the past, including users using Rider or Resharper. As far as I'm aware, it's all documented in the contributing guidelines. From what I've seen in other repositories, it's quite common that the cibuild runs more checks than what a typical developer runs locally, however if you want to run them locally, you can.

What do you propose that would improve the experience?

[verdie-g]

Should I always expect a suggestion in the IDE for a rule that would fail in the CI?

In this example I didn't get any feedback from the IDE but the inspection failed with

Warning: test\OpenApiTests\Headers\HeaderTests.cs:59 LocalFunctionCanBeMadeStatic: Local function 'AssertEtag' can be made static

[bkoelman]

In this example I didn't get any feedback from the IDE but the inspection failed with

Warning: test\OpenApiTests\Headers\HeaderTests.cs:59 LocalFunctionCanBeMadeStatic: Local function 'AssertEtag' can be made static

It's a bug in our ruleset configuration, #1473 fixes that.

[verdie-g]

Got it. I'll keep this issue open a little longer to have a better understanding about the cases where I wasn't able to catch the issues before pushing them.

[verdie-g]

Warning: benchmarks\QueryString\QueryStringParserBenchmarks.cs:92 MemberCanBePrivate.Global: Property 'QueryString' can be made private
Warning: benchmarks\QueryString\QueryStringParserBenchmarks.cs:93 MemberCanBeInternal: Property 'Query' can be made internal
Warning: benchmarks\Tools\FakeRequestQueryStringAccessor.cs:14 UnusedMember.Global: Method 'SetQueryString' is never used

Should these also be added as suggestion in the rulesets?

[bkoelman]

No, because they cause intermediate warnings while writing code.

Actually, I don't understand your point. What do you mean exactly?