Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

Enable code analysis all rules. #350

Merged
merged 3 commits into from Jan 5, 2017
Merged

Enable code analysis all rules. #350

merged 3 commits into from Jan 5, 2017

Conversation

XhmikosR
Copy link
Contributor

@XhmikosR XhmikosR commented Jan 5, 2017

Fixes #341

@whoisj: this isn't supposed to go in like that; check out my branch locally, run build with analysis, see which warnings you want disabled and push to my branch. When you are ready you can merge it :)

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Jan 5, 2017

I added a custom ruleset where we can disable rules globally. I went ahead and disable a few spelling ones. You can use that to disable more rules globally.

@whoisj
Copy link
Contributor

whoisj commented Jan 5, 2017

Thanks for this. Please see the tip commit on https://github.com/whoisj/Git-Credential-Manager-for-Windows/tree/analysis-all-rules, I think that's what we need.

Did you want to cherry-pick my single commit, or have me create a PR with your commits. You've done the initial work, so I'll let you decide. 😄

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Jan 5, 2017 via email

@whoisj
Copy link
Contributor

whoisj commented Jan 5, 2017

Ahh pushed then, thanks.

@whoisj whoisj merged commit 0bc9b5c into microsoft:master Jan 5, 2017
@XhmikosR XhmikosR deleted the analysis-all-rules branch January 5, 2017 21:01
@XhmikosR
Copy link
Contributor Author

XhmikosR commented Jan 5, 2017

@whoisj: wow, huge patch! Congrats!

BTW there are still a few errors https://ci.appveyor.com/project/whoisj/git-credential-manager-for-windows/branch/master/messages

On a side note, should we make the analyzer warnings fail the build or keep the current behavior?

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Jan 5, 2017

Also, might be worth moving the inline suppressions to the individual GlobalSuppressions.cs files so that everything is organized.

@whoisj
Copy link
Contributor

whoisj commented Jan 5, 2017

keep the current behavior

Keep the current, I'd rather not have a broken build due to "style" issues.

moving the inline suppressions to the individual GlobalSuppressions.cs

I'm of two minds here, neither option is actually great. Local suppression is easier to understand, edit, update, remove, etc. Global suppression is cleaner read and easier to find what is suppressed.

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Jan 6, 2017

It's your call ofc, I just feel that having everything in one place might be better for organization.

@whoisj whoisj modified the milestone: v1.9.0 Mar 4, 2017
@whoisj whoisj added the build-related This issue or pull-request is related to building the solution, either in CI or locally. label Mar 10, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
build-related This issue or pull-request is related to building the solution, either in CI or locally.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants