-
Notifications
You must be signed in to change notification settings - Fork 1k
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
base: develop
Are you sure you want to change the base?
Add editorconfig #1482
Conversation
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: |
An exclusion can be added to ignore the
They also have a list of fairly prominent projects using https://github.com/dotnet/corefx/blob/master/.editorconfig |
@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 Also, should we remove the comments? I find the property names sufficient and the comments just end up repeating it. |
@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/ |
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 :) |
@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 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? |
@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 |
dotnet_style_qualification_for_method = false:suggestion | ||
dotnet_style_qualification_for_property = false:suggestion | ||
|
||
[/vendor/nuget/**.cs] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
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.