Treat warnings as errors

[TysonMN]

master is warning free, so now would be a good time to have the automated builds start treating warnings as errors.

When I tried to do this with my project at work, I was blocked by https://github.com/dotnet/core-sdk/issues/1708. I don't know any workaround.

[cmeeren]

You're right, thanks for the reminder!

[TysonMN]

You're right, thanks for the reminder!

No problem; you're welcome.

I think this is better than never treating warnings as errors. However, I would prefer two changes compared to what you have done.

1. Treat warnings as errors for all projects, and
2. treat warnings as errors only during automated (appveyor) builds.

I think both are achieved by removing the line you added and adding /WarnAsError to the end of the call to dotnet build in the YAML file.

I am not certain that allowing warnings (not as errors) for local developer builds increases their productivity, but that is my current hypothesis. I feel like it is a compromise of the standard static vs dynamic arguments.

[cmeeren]

Treat warnings as errors for all projects

Agreed.

Treat warnings as errors only during automated (appveyor) builds.

I am more productive with warnings as errors locally, too. I always enable it. Warnings are too easy to overlook. With the change I did, I think I now have it enabled for 100% of my projects, both OSS and work-related.

Also, I like to have as much parity between CI and local as possible.

[TysonMN]

Treat warnings as errors for all projects

Agreed.

So that means the line you added needs to be added to every project file, right?

And I am fine with also treating warnings as errors locally. In fact, it should make for a fun experiment to see if I perceive any difference in my productivity as I switch between working on Elmish.WPF and working on my project at work.

[cmeeren]

So that means the line you added needs to be added to every project file, right?

Yes, as I just did in 2f7fc9f :)

(I figured warnings as errors was a simple enough change to commit directly, but in hindsight, I probably should have created a PR to allow for a proper review.)

And I am fine with also treating warnings as errors locally. In fact, it should make for a fun experiment to see if I perceive any difference in my productivity as I switch between working on Elmish.WPF and working on my project at work.

Yep. Though personally my focus isn't on "productivity" in any measurable sense; it's more that I just appreciate being forced to cover all pattern match cases etc., because in 99% of cases, there really is no reason to have warnings in F# - they indicate something sub-optimal or outright wrong with your code.

[TysonMN]

The (1%?) of the cases in which I think it is ok to have warnings is when I am actively in the middle of development.

I feel like it is a compromise of the standard static vs dynamic arguments.

I think the advantage of a dynamic language is not having to "fight" the compiler. Of course the disadvantage is not being able to "lean" on the compiler.

I see a similar contrast in TDD's red-green-refactor cycle. During the green step, the perfectionist in me tries to relax while the pragmatist in me does whatever necessary to obtain a correct implementation. When the pragmatist is done, the perfectionist starts the refactor step to improve names, designs, efficiencies, and fix all warnings.

I think the pull request review process is similar. By the time a PR is created, the green step is usually complete. In my experience, the changes during PR are mostly of the "factor" type. I certainly want all warnings fixed by the time a PR is merged. That is why I see it as sufficient to only treat warnings as errors in automated builds that gate PR completion.