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

Add editorconfig #1482

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

Conversation

gojanpaolo
Copy link
Contributor

@gojanpaolo gojanpaolo commented May 7, 2019

Fixes #1478

@shiftkey @Thieum I added the .editorconfig generated by IntelliCode to have a starting point. I think it might be a better idea to start from this instead of manually extracting it from the project as it has a lot of inconsistencies.

Unfortunately, IntelliCode can only generate C# code style. My next task is to add C++.

We can then format all files after the initial .editorconfig is complete.
Here's all the files formatted using this .editorconfig.
gojanpaolo/Squirrel.Windows@add-editorconfig...gojanpaolo:format-all-files
I'll keep it updated it as we go along.

@Thieum
Copy link
Contributor

Thieum commented May 7, 2019

Should we consider and apply the style to the Nuget.Core project as it's some sort of external dependencies? Parsing the code to check for the review, it became clear that the Nuget.Core project has a really different style than the Squirrel and Tests projects.

Generated both versions to see the differences:

Squirrel.editorconfig.txt
NuGet.Core.editorconfig.txt

@amaitland
Copy link

An exclusion can be added to ignore the vendor folder, see https://stackoverflow.com/questions/30310396/possible-to-ignore-exclude-file-folder-from-editorconfig

root = true and a Default Settings section should be added. The main https://editorconfig.org/ has an example (as I'm sure everyone is aware).

They also have a list of fairly prominent projects using .editorconfig which is a great reference at https://github.com/editorconfig/editorconfig/wiki/Projects-Using-EditorConfig

https://github.com/dotnet/corefx/blob/master/.editorconfig
https://github.com/dotnet/roslyn/blob/master/.editorconfig
https://github.com/aspnet/AspNetCore/blob/master/.editorconfig

@gojanpaolo
Copy link
Contributor Author

gojanpaolo commented May 7, 2019

@amaitland added root = true and default settings. :) I think we should apply a different .editorconfig on the NuGet.Core project as @Thieum suggested so we have a code style in it too in case the code needs to be updated in the future.

btw, it looks like C++ has limited support for .editorconfig (first-time dealing with C++ here). I was trying to find like new_line_before_open_brace but there isn't any. I believe the default settings should cover most of C++ style and then override some like indent_style = tab.

Also, should we remove the comments? I find the property names sufficient and the comments just end up repeating it.

@Thieum
Copy link
Contributor

Thieum commented May 7, 2019

@gojanpaolo for C++, we could go the clangformat way which is supported by VS as documented here: https://devblogs.microsoft.com/cppblog/clangformat-support-in-visual-studio-2017-15-7-preview-1/

@gojanpaolo
Copy link
Contributor Author

Ah! that's why I don't get anything when I googled "C++ EditorConfig". good to know. thanks! Will study and add it later tonight :)

@gojanpaolo
Copy link
Contributor Author

@Thieum @amaitland it looks like NuGet.Core is a git submodule and points to https://github.com/paulcbetts/NuGet . So updating it is technically out of scope of this repository thus we won't have a separate editorconfig for it as previously discussed.

Initial commit.
I used FxHelper.cpp as reference.
@gojanpaolo gojanpaolo marked this pull request as ready for review May 8, 2019 02:42
@Thieum
Copy link
Contributor

Thieum commented May 8, 2019

@gojanpaolo if we exclude Nuget.Core, did you consider the differences with what Intellicode generates only for Squirrel? https://github.com/Squirrel/Squirrel.Windows/files/3150765/Squirrel.editorconfig.txt

Maybe we should bring the editorconfig down to the project level, to avoid having warnings in the nuget.core project from a solution level editorconfig?

@gojanpaolo
Copy link
Contributor Author

gojanpaolo commented May 9, 2019

@Thieum I tried generating using the Squirrel solution excluding NuGet.Core and C++ projects and it matches the latest in this PR. I think we may benefit in the long term having a single code style for all C# squirrel project. :)

Also committed override for nuget code. This would get rid of the warnings for that project

@shiftkey shiftkey self-assigned this May 24, 2019
dotnet_style_qualification_for_method = false:suggestion
dotnet_style_qualification_for_property = false:suggestion

[/vendor/nuget/**.cs]
Copy link
Contributor

@shiftkey shiftkey May 24, 2019

Choose a reason for hiding this comment

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

@gojanpaolo I think the rules below also need to handle cpp files inside vendor/nuget/. When I ran eclint fix **/*.cpp to test out what this would detect it triggered a lot of whitespace changes (in project templates that live alongside the source).

Would that be possible to add in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shiftkey We can by doing this [/vendor/nuget/**.{cs,cpp}]
but I don't see cpp files inside vendor/nuget/

I ran eclint fix **/*.cpp and the whitespace changes were on files inside the src folder and it's because of the default settings in the .editorconfig

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.

add a simple editorconfig to the project
4 participants