Coding style, static code analysis, and versioning

[Corniel]

Framework version

The current target framework is .NET 6. Its support will be dropped end of this year. I would suggest to add .NET 8 asap, and drop .NET 6 support at the moment a breaking change has to be made.

Language version

The current language version is 10. I hope we can upgrade to C# 12. There are so many nice features that could be benefited from.

Warning as error

In the build properties, WarningAsError has been set to true. Although I understand the reasoning of that setting, I strongly believe that is counter productive. You want to be notified that you have a TODO, but it should not make you build fail. Without changing the WarningAsError locally, I can not even build the current solution, as my IDE (Visual Studio) triggers some external analyzers (including a spelling checker) that result in build errors.

Static code analysis

I would love to add:

• Sonar Analyzers (you can always have a debate on individual rules).
• QW0003: Decorate pure functions
• QW0005: Seal concrete classes unless designed for inheritance
• QW0007 - Use file-scoped namespace declarations

Editorconfig

The readability of the editor config can improved by adding a comment about the rule behind its definition. Most people - me included - do not now the identifiers of those analyzers by hart.

dotnet_diagnostic.S3218.severity = warning # Inner class members should not shadow outer class "static" or type members
dotnet_diagnostic.S3257.severity = warning # Declarations and initializations should be as concise as possible
dotnet_diagnostic.S3776.severity = warning # Cognitive Complexity of methods should not be too high
dotnet_diagnostic.S6354.severity = warning # Use a testable (date) time provider instead

As a side note, I personally really dislike RCS1008, as using var helps keeping code short and aligned. Can this be reconsidered?

Long story short, I love this project, and would like to keep participating. I think that this suggestions could make working with it even better.

[phmonte]

Hi @Corniel, thank you very much for contributing, I’m very happy!

Framework version and Language version

.NET 8.0 and language 12.0
I have no doubt that we really needed to do this update asap, I'm already looking at your PR.

WarningAsError

I also agree that it's not interesting.

Static code analysis

• Sonar Analyzers
  I would very much like;

• QW0003: Decorate pure functions
  I confess that I had a little doubt about the cost/benefit;

• QW0005: Seal concrete classes unless designed for inheritance
  I confess that I had a little doubt about the cost/benefit;

• QW0007 - Use file-scoped namespace declarations
  I find it interesting;

Editorconfig

I see sense in all the items and changing the editor.config

RCS1008

I also don't like RCS1008, I understand we can change it, but I don't believe it would be a priority asap like the .NET version.

PR

I've already seen your PR and didn't find any points of attention, thank you very much ❤️ .

I would like to download your branch locally and do some tests to try to identify possible break changes (I can't measure how faithful the tests are to identify possible problems when changing the .NET version).

[phmonte]

Hello @daveaglick, about dotnet version updates, do you remember anything relevant that we should look at?

[daveaglick]

Not really, just that I'm slow at them :)

The one part to watch out for is Buildalyzer.Logger. Since that gets injected into the build to be analyzed as a MSBuild logger, and different MSBuild version have some subtle breaking changes in terms of methods moving around and stuff, I generally try to keep that one as low a target as possible. That can get tricky though because it also needs to reference Microsoft.Build.Utilities.Core in order to have all the events that it hooks. There's a little bit of a dance in keeping it current enough that it can still be added as a logger to builds from newer SDKs while not incrementing it so much that builds with older SDKs break.

That said, Buildalyzer itself (the library that consumers call and use) isn't so tied so it can increment the build target a bit more easily.

[phmonte]

Thank you very much, I will try to do some tests following the instructions(Buildalyzer.Logger).

[Corniel]

* QW0003: Decorate pure functions
  I confess that I had a little doubt about the cost/benefit;

The cost is low, if you might wonder. Ideally, a library like Buildalizer should mainly contain [Pure] functions. In my experience, the rule has benefits for packages like Buildalyzer, but using it for your web application would be a waist of time.

* QW0005: Seal concrete classes unless designed for inheritance
  I confess that I had a little doubt about the cost/benefit;

Also low costs. QW0003 also comes with a code fix, so sealing classes is just a ctrl + .. The key here is that guaranteeing the right behavior when people start inheriting classes defined in Buildalyzer is hard in most cases. And that is exactly what the rule is about: as long as you did not define a virtual or protected member, the class apparently was not designed to be inheritable. This rule is enabled at most of projects at our company, and it hardly occurs that people and an [Inheritable] attribute on top of their classes, which indicates for me that the rule works like it should. My suggestion would be to try it out, and we then can decide if it brings the value I think it does.

I would like to download your branch locally and do some tests to try to identify possible break changes (I can't measure how faithful the tests are to identify possible problems when changing the .NET version).

Well, I did not change any package of the assemblies shipped, and I suggest not to do that for that PR anyway.

[Corniel]

A thing that I forgot to mention, but I think is worth enabling, is nullability. I enabled it on the files I created in my open PR's, but I think it worth applying it - eventually - on the full code base. But obviously, applying it is not trivial, and should not be hurried.

[phmonte]

Hello @Corniel, I was just running your branch locally, apparently everything works perfectly.
Sorry for the delay, but at the moment I'm being a little more cautious with big changes, but this update is necessary and we're going to do the merge at the beginning of the week, okay?
I see that you are very active in this repository and I would like to share that I intend to evaluate all open issues and resolve or close as many as possible, then I would like to create a simple and public backlog about developments, bugs and new features.
If you have suggestions, please let me know, your help and opinion are very welcome.

[phmonte]

@daveaglick Microsoft.Build.Utilities.Core is very outdated, the good news is that I updated and did some tests locally and it seems ok, but we are not going to update at this time.
I was surprised that the update didn't break anything, but it was a quick test that if it is actually updated, it deserves more attention.

[daveaglick]

Yeah, it scared me, but mostly because of my lack of time to troubleshoot if something went sideways once released. Probably not a bad idea to bump that one given how old it is once you've got a good handle on any risks and do some more testing.

[phmonte]

@Corniel .NET version merged.
As soon as I gain access to the nuget I will publish the package.

[Corniel]

@phmonte I would argue that there is no hurry in pushing a new version. The only difference is the extra .NET 8 target.

[phmonte]

@Corniel I saw that there are some changes you want to make, are there any more besides the open PRs?

[Corniel]

@phmonte I would like to slowly move forward. But all with baby steps.

[phmonte]

@Corniel are there any more questions? If not, I'll close this issue.

[Corniel]

Yes, the static code analysis warnings should still be further revisited if you ask me.