Unused values as warnings

[TysonMN]

First commit modifies fsproj files to turn unused values into build warnings. The second commit deletes one unused line of test code. The third commit renames all unused values to _.

[cmeeren]

Thanks! I'm a bit split on this.

I can see how it may be useful to know at a glance whether a value is used or not. VS already displays unused values in a muted colour, so in the IDE it's clear which values are unused.

On the other hand, I have often found it equally (if not more) nice to know at a glance what a parameter actually is, even if unused, and names help with this. For example, it becomes easier to make use of a parameter later when needed.

It is not obvious to me that one always trumps the other, and I'm therefore not sure that it should be a warning.

For example, in the samples, I think I generally prefer names: the samples are deliberately intended to help new users, and I think (at least in bindings) they might appreciate (slightly) descriptive names more than knowing whether they're unused (which, again, VS makes clear anyway).

In any case, given that I don't think it's generally a given which option to prefer, I have never turned on --warnon:1182. (This is also combined with the fact that I usually always turn on <WarningsAsErrors />, forcing me to fix warnings.)

In summary, I'm not sure I'd like to turn the warning on.

(I have not yet gone through and considered each individual parameter renaming.)

What do you think, in light of the above reasoning?

[TysonMN]

...I have often found it equally (if not more) nice to know at a glance what a parameter actually is, even if unused, and names help with this. For example, it becomes easier to make use of a parameter later when needed.

I think code should be optimized for reading since that is the typical case. If someone is going to write new code in that area, then they can look at the type to quickly determine "what a parameter actually is".

In this case of CmdParam, the type of the parameter is just obj. For lambda functions, the convention I see (both in this project and beyond) is to have the identifier be the first letter of the corresponding type. So for this case, I agree that replacing p with _ makes things significantly less clear when not reading. However, I think the fix is to give a proper type to the parameter. See dotnet/installer#146.

[TysonMN]

In any case, given that I don't think it's generally a given which option to prefer, I have never turned on --warnon:1182. (This is also combined with the fact that I usually always turn on <WarningsAsErrors />, forcing me to fix warnings.)

I agree that <WarningsAsErrors /> is a good idea, but sadly it is currently broken :(
https://github.com/dotnet/core-sdk/issues/1708

[TysonMN]

For example, in the samples, I think I generally prefer names...

A simple compromise would be to only add --warnon:1182 in non-sample projects.

[cmeeren]

You make good sense. I can agree that code should be optimized for reading. I can accept the changes to the main project.

I can also accept most of the changes to the samples. Apart from the --warnon:1182 you mentioned, there are actually just a single change I'd really like reverted. I've left a comment.

Also, instead of <OtherFlags>$(OtherFlags) --warnon:1182</OtherFlags> I think there may be a WarnOn element of sorts that can be used to add codes more directly (though I can't find it right now, so I may be mistaken). Could you have a quick look? (Don't spend your entire evening on it.)

[TysonMN]

I would prefer to squash some commits before merging

[cmeeren]

Let me know when you're ready!

[TysonMN]

Also, instead of <OtherFlags>$(OtherFlags) --warnon:1182</OtherFlags> I think there may be a WarnOn element of sorts that can be used to add codes more directly (though I can't find it right now, so I may be mistaken). Could you have a quick look? (Don't spend your entire evening on it.)

This is the only way that I know how to do it in the project file.

I have found this part of devops rather confusing. On the one had, there is very clear information about what options fsc, the F# compiler, accepts. On the other hand, us developers call commands like dotnet build that have their own default arguments, extract arguments from the project files, and eventually call the F# compiler or one of the C# compilers. What seems to be missing is documentation specifying all of the translations from dotnet build to fsc.

[TysonMN]

Ready :)

[cmeeren]

Thanks a lot!