Skip to content
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

StyleCop fixes #3831

Merged
merged 82 commits into from
Jun 15, 2020
Merged

StyleCop fixes #3831

merged 82 commits into from
Jun 15, 2020

Conversation

bdukes
Copy link
Contributor

@bdukes bdukes commented Jun 9, 2020

Most of the auto-fixes

@bdukes bdukes added this to the 9.6.2 milestone Jun 9, 2020
@valadas
Copy link
Contributor

valadas commented Jun 9, 2020

Odd, there are random charaters missing here and there:

image

@bdukes
Copy link
Contributor Author

bdukes commented Jun 9, 2020

😦 Looks like these auto-fixes aren't without their bugs… Ugh.

@bdukes bdukes changed the title Style cop fixes (spacing) StyleCop fixes Jun 10, 2020
@bdukes
Copy link
Contributor Author

bdukes commented Jun 10, 2020

I was able to find a way to automate applying most of the fixes. It's not 100%, and a few of the documentation fixes are weird (e.g. SA1642). I'm going to try another round of automatically applying fixes and pinpoint the ones that cause compile problems.

bdukes added 23 commits June 11, 2020 20:59
bdukes added 13 commits June 12, 2020 11:44
@bdukes
Copy link
Contributor Author

bdukes commented Jun 12, 2020

Alright, build succeeded. 55,464 warnings instead of 194,461 warnings on the previous build. Also shaved 7 minutes off the build.

I'm going to manually apply/fix some of the auto-fixes that caused build errors. Y'all can merge this now and I'll push those changes as a separate PR, or I can push to this PR if nothing's moved on it before I'm done.

Copy link
Contributor

@valadas valadas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a "spot check", it looks fine to me and I resolved 2 small conflicts. Looks good to me

@valadas
Copy link
Contributor

valadas commented Jun 13, 2020

Hmm, we have a new error, not sure if caused by my fixing of conflicts, they where pretty basic to resolve...

@bdukes
Copy link
Contributor Author

bdukes commented Jun 13, 2020

Thanks @valadas! I've just pushed a change which should fix it. One side moved the using statements inside the namespace and the other side added a new using statement, and the resolution left out the new using statement.

@bdukes
Copy link
Contributor Author

bdukes commented Jun 13, 2020

Also, with the SA1200 change, we're down to 36,883 warnings.

@valadas
Copy link
Contributor

valadas commented Jun 15, 2020

Oh, sorry, my bad :)

/azp run

@valadas
Copy link
Contributor

valadas commented Jun 15, 2020

/azp run

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@valadas
Copy link
Contributor

valadas commented Jun 15, 2020

Cool, it builds :) Can we get a second approver @dnnsoftware/approvers

@mitchelsellers
Copy link
Contributor

Just confirming before I do this, we are good to merge this right? @valadas @bdukes

@bdukes
Copy link
Contributor Author

bdukes commented Jun 15, 2020

Yessir

@mitchelsellers mitchelsellers merged commit cc61549 into dnnsoftware:develop Jun 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants