Skip to content

Reformat code according to EditorConfig rules #11681

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.

Sign up for GitHub

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

Jump to bottom
Merged
TravisEz13 merged 2 commits into from
Jan 31, 2020
Merged

Reformat code according to EditorConfig rules #11681

TravisEz13 merged 2 commits into from
Jan 31, 2020

Conversation

@xtqqczze
Copy link
Contributor

@xtqqczze xtqqczze commented Jan 26, 2020 •
edited
Loading

PR Summary

Reformat source code with dotnet/format according to rules in the .editorconfig file.

dotnet tool install --global dotnet-format --version 3.1.37601
dotnet-format

The following rules were disabled as they resulted in a high number of changes:

csharp_preserve_single_line_statements = false
csharp_new_line_before_open_brace = all

To exclude regressions, we could create a new static test:

# Terminates with a non-zero exit code if any files were formatted
dotnet-format --check --dry-run

PR Context

PR Checklist

@ghost ghost assigned TravisEz13 Jan 26, 2020
@xtqqczze xtqqczze mentioned this pull request Jan 26, 2020
Merged
14 tasks
iSazonov
iSazonov reviewed Jan 26, 2020
View reviewed changes
Copy link
Collaborator

@iSazonov iSazonov left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@xtqqczze Please revert changes in .editorconfig. They was needed only locally to allow you to format but we should keep these options as they correspond to our code convention.
(For static test we could use another config file with fewer rules.)

Which rules would you like to see added?

I see many rules in .Net Core and Roslyn .editorconfig-s and we could review them and add some ones to our config. When I added these rules last time I had to remove many because there were a lot of problems. Now the code is cleaner and we can add more rules.

Also please see CodeFactor issues. We need to adjust CodeFactor or formatting options to resolve them.

@xtqqczze
Copy link
Contributor Author

xtqqczze commented Jan 26, 2020

CodeFactor issues are violations of SA1008 due to added space after the delegate keyword. Rule documentation states an opening parenthesis can be preceded by whitespace when it is preceded by "certain C# keywords" but is ambiguous as to whether or not this applies to the delegate keyword. There is an open issue dotnet/roslyn#3257 concerning this behaviour.

We can apply a refactoring to replace the anonymous methods with delegate keyword with lamba expressions and avoid the rule violations. This would be a workaround but is also a recommended refactoring.

@xtqqczze xtqqczze mentioned this pull request Jan 27, 2020
Merged
14 tasks
@xtqqczze
Copy link
Contributor Author

xtqqczze commented Jan 27, 2020 •
edited
Loading

@iSazonov This PR disables those rules because it is apparent the code is not already in the prescribed style. We could open a new issue to discuss which rules we want for our code convention?

@xtqqczze xtqqczze force-pushed the patch-dotnet-format branch from 75aef8b to 3052d56 Compare January 27, 2020 03:07
@xtqqczze
Copy link
Contributor Author

xtqqczze commented Jan 27, 2020

rebased to restart CI

@iSazonov
Copy link
Collaborator

iSazonov commented Jan 27, 2020

This PR disables those rules because it is apparent the code is not already in the prescribed style.

We have over 50000 CodeFactor issues but we do not disable the rules because we don't want add new issues.

We could open a new issue to discuss which rules we want for our code convention?

We already have Code Conventions. This allows us add any rules to suggest/force the Code Conventions.

@xtqqczze xtqqczze changed the title [WIP] Reformat code with dotnet-format according to EditorConfig rules Reformat code according to EditorConfig rules Jan 27, 2020
@iSazonov
Copy link
Collaborator

iSazonov commented Jan 30, 2020

@xtqqczze Please resolve merge conflict.

@xtqqczze xtqqczze force-pushed the patch-dotnet-format branch from 18570c9 to e2fea92 Compare January 30, 2020 10:16
@xtqqczze
Copy link
Contributor Author

xtqqczze commented Jan 30, 2020

@iSazonov rebased and removed commit "Remove duplicate semicolons" This should be part of a seperate PR as the change is unrelated to using the dotnet-format tool.

@xtqqczze
Copy link
Contributor Author

xtqqczze commented Jan 30, 2020

@iSazonov We should make changes to rules in .editorconfig before creating a new dotnet-format PR.

@iSazonov iSazonov added the CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log label Jan 30, 2020
@iSazonov iSazonov added this to the 7.1.0-preview.1 milestone Jan 30, 2020
@TravisEz13
Copy link
Member

TravisEz13 commented Jan 30, 2020

Please resolve merge conflicts.
Such a large PR may be difficult to get merged.

@xtqqczze xtqqczze force-pushed the patch-dotnet-format branch from e2fea92 to c8ffda3 Compare January 30, 2020 21:15
@xtqqczze
Copy link
Contributor Author

xtqqczze commented Jan 30, 2020

@TravisEz13 rebased

@xtqqczze xtqqczze force-pushed the patch-dotnet-format branch from c8ffda3 to edab15a Compare January 30, 2020 23:15
@xtqqczze
Copy link
Contributor Author

xtqqczze commented Jan 30, 2020

@TravisEz13 rebased, again

iSazonov
iSazonov reviewed Jan 31, 2020
View reviewed changes
@xtqqczze xtqqczze force-pushed the patch-dotnet-format branch from edab15a to 83106fd Compare January 31, 2020 11:18
@xtqqczze
Copy link
Contributor Author

xtqqczze commented Jan 31, 2020

PowerShell-CI-static-analysis failures are in markdown-links and are unrelated.

@TravisEz13 TravisEz13 merged commit a34d0f3 into PowerShell:master Jan 31, 2020
@xtqqczze xtqqczze deleted the patch-dotnet-format branch February 1, 2020 12:23
@ghost
Copy link

ghost commented Mar 26, 2020

🎉v7.1.0-preview.1 has been released which incorporates this pull request.:tada:

Handy links:

@xtqqczze xtqqczze mentioned this pull request Aug 22, 2020
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov iSazonov iSazonov approved these changes

@TravisEz13 TravisEz13 TravisEz13 approved these changes

@adityapatwardhan adityapatwardhan Awaiting requested review from adityapatwardhan

@anmenaga anmenaga Awaiting requested review from anmenaga

@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw daxian-dbw is a code owner

@SteveL-MSFT SteveL-MSFT Awaiting requested review from SteveL-MSFT

Assignees

@TravisEz13 TravisEz13

Labels

CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log

Projects

None yet

Milestone

7.1.0-preview.1

Development

Successfully merging this pull request may close these issues.

3 participants

@xtqqczze @iSazonov @TravisEz13