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

Style checking for core #7516

Merged
merged 1 commit into from
Sep 9, 2019

Conversation

chidozieononiwu
Copy link
Member

Move .editorconfig to Repo Root.
Run dotnet format for Azure.Core to fix style errors.
Turn on Style Cop for Azure.Core
Fix Style Cop errors for Azrue.Core

Copy link
Contributor

@pakrym pakrym left a comment

Choose a reason for hiding this comment

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

Looks good except the newline for single line blocks.

@pakrym
Copy link
Contributor

pakrym commented Sep 6, 2019

Also, do we want .editorconfig at the root? it would affect all projects both track1 and management

@chidozieononiwu
Copy link
Member Author

Also, do we want .editorconfig at the root? it would affect all projects both track1 and management

I discussed this with @heaths and he thought it won't matter since the rules are only applied when one runs dotnet format or used the editorconfig plugin for vscode. For the various other editors that have native support for editorconfig I guess it will still be an issue. The other options will be to keep .editorconfig inside the service folder.

@pakrym
Copy link
Contributor

pakrym commented Sep 6, 2019

Just doing format document in VS would apply our styles to their code.

@chidozieononiwu
Copy link
Member Author

Just doing format document in VS would apply our styles to their code.

Maybe we should keep .editorconfig inside the package folders since there seems to be no way to add exclusion or scope in the root .editorconfig file

@pakrym
Copy link
Contributor

pakrym commented Sep 6, 2019

Maybe we should keep .editorconfig inside the package folders since there seems to be no way to add exclusion or scope in the root .editorconfig file

I think it would be the safest option for now, we might need a script to syncronize them later.

@heaths
Copy link
Member

heaths commented Sep 6, 2019

Another option might be to try path wildcard matching for styles, like:

[sdk/*/Azure.*/**]
# General files

[sdk/*/Azure.*/**.cs]
# C# styles

@heaths
Copy link
Member

heaths commented Sep 6, 2019

What exactly didn't work? It still applied styles to track 1 sources?

@chidozieononiwu
Copy link
Member Author

What exactly didn't work? It still applied styles to track 1 sources?

Yes, it still runs style check on Track 1, and Management.
It seems that overides can be applied using the method you suggested. But that will require overriding too many paths and you have to do it one at a time which will mean a bulky .editorconfig file.

@heaths
Copy link
Member

heaths commented Sep 6, 2019

I just tried an experiment where I put the following content in .editorconfig in the repo root and it worked, i.e. *.cs files under sdk/keyvault/Azure.* used tabs (ick), but when I even just opened an individual file from sdk/keyvault/Microsoft.* it still used spaces.

root = true

[sdk/*/Azure.*/**]
indent_style = space
indent_size = 4
trim_trailing_whitespace = true
insert_final_newline = true

[sdk/*/Azure.*/**/*.cs]
dotnet_sort_system_directives_first = true
dotnet_separate_import_directive_groups = false

@chidozieononiwu
Copy link
Member Author

I just tried an experiment where I put the following content in .editorconfig in the repo root and it worked, i.e. .cs files under sdk/keyvault/Azure. used tabs (ick), but when I even just opened an individual file from sdk/keyvault/Microsoft.* it still used spaces.
root = true

[sdk//Azure./**]
indent_style = space
indent_size = 4
trim_trailing_whitespace = true
insert_final_newline = true

[sdk//Azure./**/*.cs]
dotnet_sort_system_directives_first = true
dotnet_separate_import_directive_groups = false

I'll try that again.

@heaths
Copy link
Member

heaths commented Sep 7, 2019

Also, what editor are you using? I used Visual Studio 2019 that supports .editorconfig natively. I'd be surprised, though, if some editor/extension supported enough of the pattern to apply to certain files but not within a directory. That would be a bug, given https://editorconfig.org documents the pattern as supported.

@chidozieononiwu
Copy link
Member Author

Also, what editor are you using? I used Visual Studio 2019 that supports .editorconfig natively. I'd be surprised, though, if some editor/extension supported enough of the pattern to apply to certain files but not within a directory. That would be a bug, given https://editorconfig.org documents the pattern as supported.

Actually I was testing by running dotnet format which applied the editorconfig rules regardless of the specified path.

@heaths
Copy link
Member

heaths commented Sep 7, 2019

In my experience, it's pretty buggy. But what's the desired result here? If we just want to get track 2 libraries on par with .editorconfig we could run dotnet format on just those initially. Any linting we should do at save/build time anyway, so perhaps something could be put into those project files (or something they import, targeting those client projects). Put another way, I don't see much point of running dotnet format more than once initially, so you could do it on those 4 services in track 2.

dir sdk/*/Azure.* -recurse -filter *.csproj | foreach { dotnet format -w $_.fullname }

After that, conforming editors would at least suggest - if not err, depending on rule severity - changes when editing files. That's all .editorconfig is good for, really. It's violations won't break the build. For that, you need Roslyn Analyzers / StyleCop (the new ones, which are RAs).

@chidozieononiwu
Copy link
Member Author

In my experience, it's pretty buggy. But what's the desired result here? If we just want to get track 2 libraries on par with .editorconfig we could run dotnet format on just those initially. Any linting we should do at save/build time anyway, so perhaps something could be put into those project files (or something they import, targeting those client projects). Put another way, I don't see much point of running dotnet format more than once initially, so you could do it on those 4 services in track 2.
dir sdk//Azure. -recurse -filter *.csproj | foreach { dotnet format -w $_.fullname }
After that, conforming editors would at least suggest - if not err, depending on rule severity - changes when editing files. That's all .editorconfig is good for, really. It's violations won't break the build. For that, you need Roslyn Analyzers / StyleCop (the new ones, which are RAs).

You are right. dotnet format is only for the initial code fixes after which I shouldn't be used. I will test again and then likely to use the paths as you suggested.

@chidozieononiwu chidozieononiwu merged commit 2adc48f into Azure:master Sep 9, 2019
@chidozieononiwu chidozieononiwu deleted the StyleCheckingForCore branch September 9, 2019 21:16
JoshLove-msft pushed a commit to JoshLove-msft/azure-sdk-for-net that referenced this pull request Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants