Remove redundant assignment

[elachlan]

Refactoring to remove redundant assignments.
https://github.com/JosefPihrt/Roslynator/blob/master/docs/analyzers/RCS1212.md

[Forgind]

I actually really like this change because I think it will make it much more obvious when we try to set a default value for something and accidentally don't overwrite it. Would you mind resolving my comments (especially with moving declarations into definitions with out)? Thanks!

[Forgind]

Looks good! I'd still like to see the patterns of the form:

<type> <name>;
<functionCall>(<param0>, <param1>, out <name>);

or equivalent translated into
<functionCall>(<param0>, <param1>, out <type> <name>);
But we can save that for later.

[elachlan]

Looks good! I'd still like to see the patterns of the form:

<type> <name>;
<functionCall>(<param0>, <param1>, out <name>);

or equivalent translated into
<functionCall>(<param0>, <param1>, out <type> <name>);
But we can save that for later.

I can do another refactor PR for that. There are a lot of changes like that so it will be easier to review in a separate issue.

[benvillalobos]

I haven't gone through everything, but here's some feedback based on what I've seen so far.

[cdmihai]

Reviewed half and then got tired and gave it up. As long as the class of issues that I commented on get fixed, looks good to me.

Since these big repetitive changes suck the life out of reviewers, maybe we should have a contribution rule where we restrain these types of changes somehow. Thinking out loud, one strategy would be to only allow them on the files touched by a PR that has a meaningful feature addition / bug fix change, or some other throttling mechanism. We could also define a set of linters to use and/or config files for them (resharper, VS, roslynator, etc)

[cdmihai]

Revert this, older version seems more readable.

[elachlan]

oldestOutputFileTime = NativeMethodsShared.GetLastWriteFileUtcTime returns "The last write time of the file, or DateTime.MinValue if the file does not exist". So we don't need to assign DateTime.MinValue. Additionally the try catch also assigns it. Same thing with candidateOutputFileTime. I've added it back in for you though.

[elachlan]

Reviewed half and then got tired and gave it up. As long as the class of issues that I commented on get fixed, looks good to me.

Since these big repetitive changes suck the life out of reviewers, maybe we should have a contribution rule where we restrain these types of changes somehow. Thinking out loud, one strategy would be to only allow them on the files touched by a PR that has a meaningful feature addition / bug fix change, or some other throttling mechanism. We could also define a set of linters to use and/or config files for them (resharper, VS, roslynator, etc)

Thanks for getting so far. I will create an issue to look at copying the Anlyzers and config from the dotnet/runtime codebase.

[cdmihai]

Thanks for getting so far. I will create an issue to look at copying the Anlyzers and config from the dotnet/runtime codebase.

That would cool, thanks! It would also be cool if we had a bot which adds a commit to every PR with the rules applied to the changed files. That would trickle the changes in a more digestible manner. Alternatively, we could also just apply everything in one humongous PR (maybe linting + formatting), and then have a few weeks of stabilization where we check that nothing regressed. And after that have a bot apply them to each PR.

[elachlan]

Thanks for getting so far. I will create an issue to look at copying the Anlyzers and config from the dotnet/runtime codebase.

That would cool, thanks! It would also be cool if we had a bot which adds a commit to every PR with the rules applied to the changed files. That would trickle the changes in a more digestible manner. Alternatively, we could also just apply everything in one humongous PR (maybe linting + formatting), and then have a few weeks of stabilization where we check that nothing regressed. And after that have a bot apply them to each PR.

Here is the issue #5655.

[Forgind]

Team triage:
Can you resolve @cdmihai's first two comments? Then we'll take it.

We're also thinking that we might use #5656 as a checklist to measure progress towards a nicely refactored repo, in which case you can put the lint ID you're addressing in the title to make it easy to find. We should have a separate team meeting to discuss what rules we want to keep and which we might discard, though, and we haven't had that yet. We'll get back to you when that happens.

[Forgind]

We agreed it wasn't necessary to have these two assignments, but it made a little clearer what was going on. If I see an arbitrary int, for instance, that tells me very little about it. Seeing that there's something called a BuildRequest, and this is an ID for a build makes that a lot clearer.

Thanks for making this change! I'm really glad you did.