Coding style, static code analysis, and versioning about buildalyzer

[phmonte]

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

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

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

[Corniel]

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

[Corniel]

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

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

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

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

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

[phmonte]

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