Add .props file to initialize all contract settings #14
Conversation
|
Here is something that I am not 100% sure. First of all, it is obviously a breaking change. Also it introduces some hidden side effects just by including the package.
Let me think about this a bit. |
| <!--===================================================================== | ||
| Runtime checking. If false, whole section is irrelevant. | ||
| ======================================================================--> | ||
| <CodeContractsEnableRuntimeChecking>False</CodeContractsEnableRuntimeChecking> |
There was a problem hiding this comment.
All these changes set unconditionally. Do you know if they are going to override whatever set in the project file?
I need to verify, but my guess is it would override.
There was a problem hiding this comment.
The .props file is imported very early in the project. Many packages do not pack .props files at all, because you cannot refer to common build variables, like for example $(IntermediateOutputPath), because those are set later. Anythings you can do is set properties unconditionally, such that they do not depend on anything but properties that you already set above them, and MSBuild intrinsics. Conditional setting is not useful, and there are not even variables you can use in conditions. Even the variables you are setting there could not have ever been set.
In the case of contracts, .props is actually the most proper place to set the defaults, as none of them depend on anything, and are just initial constant values. We are just reproducing the constants that were in the past put in a project the first time code contracts page was opened, and either static or dynamic checking enabled.
More important, the extinct VS extension always put all of them into the project, and the build scripts were never written to support a case where some of these variables would be left unset. So, unless they are initialized in the .props file, it's the user's burden to define all and every single one of these properties in their project. Since most users do not understand even half of them (I do not, for one), this becomes just an unnecessary "copy and paste some magical block" job.
The above also means this is also not a breaking change. If someone's project worked normally, it either was because (a) they copied all of code contract settings, as they were supposed to do, or (b) they did not copy all of them and they were also lucky, relying on undefined behavior. In the case (b) they can be broken, but this is akin to relying on uninitialized variables set some particular way: one compiler zeroes them out, and in the next version it does not.
Hope I convinced you. :)
There was a problem hiding this comment.
Thanks, you definitely convinced me! :)
I only want to test the scenario when you have multiple .props included OR when you have Directory.Build.Props file. I want to make sure that the settings there are respected. Give me a day to test this out.
There was a problem hiding this comment.
Absolutely. And a good point, since Directory.Build.props is imported earlier than packaged .props. This is by design, as I understand; this is where you can e. g. set ExcludeRestorePackageImports to true to prevent imports at all.
So... I do not know. There are probably 50 people out there using the package. Maybe even a 100? I can add the guards if you think it's worth it (I do not, you see). But this is your call. Just LMK.
There was a problem hiding this comment.
So yes, the settings set in Directory.Build.props are being overwritten in this file.
I'd like to add conditions for each parameter in the form:
<CodeContractsXXX Condition=" '$CodeContractsXXX' == '' ">YYY</CodeContractsXXX>
I can merge this PR and change it afterwards or you could add them in here. Just let me know what's the best for you.
There was a problem hiding this comment.
Sure, I'll add them, no big deal.
|
Done. Defaults like look weird, but I left them in uniformly, for the sake of documentation. |
| '$(CodeContractsUseBaseLine)' == ''">False</CodeContractsUseBaseLine> | ||
| <!-- Baseline file --> | ||
| <CodeContractsBaseLineFile Condition=" | ||
| '$()CodeContractsBaseLineFile' == ''" /> |
|
D-oh! And MSBuild was happy with that! |
|
Thanks a lot! |
| <!--===================================================================== | ||
| CODE CONTRACT DEFAULTS. | ||
| All defaults are set as they were by the late Visual Studio extension, | ||
| except for enabling the static checker by default (otherwise the user |
There was a problem hiding this comment.
Some users might want runtime checking but not static checking
There was a problem hiding this comment.
Correct, and they can set the options as they want.
In Visual Studio, all switches are always saved to the project file. This creates unnecessary clutter, especially ugly with the new compact SDK-style projects. I am adding a .props file that initializes all switches to the same defaults as the unfortunately late VS extension did, except for enabling static checking by default. (The theory is that if the user did not want contract checking, they would not install the package in the first place.)