-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Initial code analyzer for Microsoft.ML, use limited StyleCop #557
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 15 commits
f5c8e04
bc3edf2
d19adb1
d98092f
1faa44c
19a4993
89362cd
90b5129
dde0ae7
ba91e6b
bc12f31
a3d703a
f27fbda
ad638eb
0b1e6b7
d3d6fbd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,7 +11,17 @@ | |
| <NoWarn>$(NoWarn);1591</NoWarn> | ||
| <WarningsNotAsErrors>$(WarningsNotAsErrors);1591</WarningsNotAsErrors> | ||
|
|
||
|
|
||
| <CodeAnalysisRuleSet>$(MSBuildThisFileDirectory)\Source.ruleset</CodeAnalysisRuleSet> | ||
| </PropertyGroup> | ||
|
|
||
| <ItemGroup> | ||
| <ProjectReference | ||
| Condition="'$(UseMLCodeAnalyzer)' != 'false'" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You might also want a project type condition
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good idea
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK I have added this as a condition to both the stylecop and project import @ericstj |
||
| Include="$(MSBuildThisFileDirectory)\..\tools-local\Microsoft.ML.CodeAnalyzer\Microsoft.ML.CodeAnalyzer.csproj"> | ||
| <ReferenceOutputAssembly>false</ReferenceOutputAssembly> | ||
| <OutputItemType>Analyzer</OutputItemType> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 😎 First time I've seen this used in practice. Very cool if it works. |
||
| </ProjectReference> | ||
| <PackageReference Condition="'$(UseStyleCopAnalyzer)' != 'false'" Include="StyleCop.Analyzers" Version="1.1.0-beta008" PrivateAssets="All" /> | ||
| </ItemGroup> | ||
|
|
||
| </Project> | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may want add a condition like
Condition="'$(UseMLCodeAnalyzer)' != 'false'"so that projects could opt out. Supporting such a condition requires this to be inDirectory.Build.targetsso that it is imported after the project body where a project might set this property.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All MSBuild properties are evaluated first, then Items are evaluated in a second pass. So it doesn't necessarily need to be in a
.targetsfile.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this was something I was curious about with how MSBuild works, and I couldn't find the documentation on this... so let me see if I have this right @eerhardt and @ericstj ... so imported projects (including implicitly imported projs like
Directory.Build.props, their properties (that is, I suppose, things inPropertyGrouptags) are evaluated before the local importing project file, but their items (things inItemGrouptags) are evaluated after the local importing project file? Is that so?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right you are. Still for sanity's sake its usually better to follow logical order.
@TomFinley here are the docs: https://msdn.microsoft.com/en-us/library/dd997067.aspx#Anchor_2. Like @eerhardt said you don't necessarily need to move it when adding the condition, but I like to do that by convention for sanity's sake. In general I try to use props files only when I need to define something that a project might need to consume itself or I need it defined before some other targets are imported. Also if you want a visual representation of the import resolution you can run /pp on the project file (msbuild myproject.csproj /pp:project.pp).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah OK... so all properties are evaluated in the order in which they occur no matter where they may be, then all items, I guess? And the
Directory.Build.props(if found) is always implicitly imported first? That begins to make some sense, I think.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Directory.Build.props (and targets) are just imported by convention in the SDK props/targets, which get imported before/after the project body respectively. /pp output shows you precisely where (which is sometimes relevant when trying to override).