Add CSharpier config #15287
Add CSharpier config #15287
Conversation
| @@ -0,0 +1,6 @@ | |||
| { | |||
|
Should it be part of the project? Does it mean someone will run the tool locally? I don't think I would like the CI to block my PR because I missed a space. (I know I don't like it when it happens in other repos, example ImageSharp). |
|
Yes, we will need to run the tool locally to reformat everything at some point. We can also add a VS Code setting so that it be applied on saving the file automatically. I wanted to see if we would adopt it or not first. |
|
Formatting with CSharpier will not be reformatted by Visual Studio afterward from what I've seen so far. |
with `dotnet csharpier .`
Skrypt
requested review from
sfmskywalker,
deanmarcussen,
kevinchalet,
hishamco,
Jetski5822,
carlwoodhouse and
jtkech
as code owners
|
This pull request has merge conflicts. Please resolve those before requesting a review. |
# Conflicts: # src/OrchardCore.Modules/OrchardCore.Admin/AdminAreaControllerRouteMapper.cs # src/OrchardCore.Modules/OrchardCore.AdminMenu/Recipes/AdminMenuStep.cs # src/OrchardCore.Modules/OrchardCore.Apis.GraphQL/ValidationRules/MaxNumberOfResultsValidationRule.cs # src/OrchardCore.Modules/OrchardCore.AuditTrail/Controllers/AdminController.cs # src/OrchardCore.Modules/OrchardCore.ContentFields/GraphQL/Fields/ObjectGraphTypeFieldProvider.cs # src/OrchardCore.Modules/OrchardCore.ContentFields/Startup.cs # src/OrchardCore.Modules/OrchardCore.ContentLocalization/Startup.cs # src/OrchardCore.Modules/OrchardCore.Contents/AuditTrail/Startup.cs # src/OrchardCore.Modules/OrchardCore.Contents/Deployment/AddToDeploymentPlan/AddToDeploymentPlanStartup.cs # src/OrchardCore.Modules/OrchardCore.Contents/Deployment/Download/DownloadStartup.cs # src/OrchardCore.Modules/OrchardCore.CustomSettings/Drivers/CustomSettingsDisplayDriver.cs # src/OrchardCore.Modules/OrchardCore.CustomSettings/Services/CustomSettingsService.cs # src/OrchardCore.Modules/OrchardCore.Google/Authentication/Configuration/GoogleOptionsConfiguration.cs # src/OrchardCore.Modules/OrchardCore.Layers/Deployment/AllLayersDeploymentSource.cs # src/OrchardCore.Modules/OrchardCore.Layers/Recipes/LayerStep.cs # src/OrchardCore.Modules/OrchardCore.Lists/Startup.cs # src/OrchardCore.Modules/OrchardCore.Media.AmazonS3/Controllers/AdminController.cs # src/OrchardCore.Modules/OrchardCore.Resources/ResourceManagementOptionsConfiguration.cs # src/OrchardCore.Modules/OrchardCore.Search.AzureAI/Startup.cs # src/OrchardCore.Modules/OrchardCore.Search.Elasticsearch/Startup.cs # src/OrchardCore.Modules/OrchardCore.Sitemaps/Startup.cs # src/OrchardCore.Modules/OrchardCore.Sms/Startup.cs # src/OrchardCore.Modules/OrchardCore.Twitter/Signin/Configuration/TwitterOptionsConfiguration.cs # src/OrchardCore/OrchardCore.ContentManagement.GraphQL/Queries/Types/TypedContentTypeBuilder.cs # src/OrchardCore/OrchardCore.Search.AzureAI.Core/Services/AzureAISearchIndexSettingsService.cs # src/OrchardCore/OrchardCore.Shells.Azure/Extensions/BlobShellsOrchardCoreBuilderExtensions.cs # src/OrchardCore/OrchardCore/Modules/Extensions/ServiceCollectionExtensions.cs
| "[csharp]": { | ||
| "editor.defaultFormatter": "csharpier.csharpier-vscode", | ||
| "editor.formatOnSave": true | ||
| } |
There was a problem hiding this comment.
Added VS Code settings
|
Yes I support this. means no more opinions about what code style to be using. I believe it can also be installed on the CI to autoformat and push back, which would be a good future improvement. 👍 |
|
I think it would be nice to create an action that runs it once a week to create a PR automatically. Side question, why not dotnet-format. It's part of the SDK and it support the editorconfig file which already have our formatting preferences. |
|
Would need to compare it with CSharpier. I believe it supports editorConfig too but maybe not the same way. Need to read documentation. I have nothing against dotnet-format I did not even know it existed! |
|
This pull request has merge conflicts. Please resolve those before requesting a review. |
|
Ok, took a look and tested dotnet-format. The difference is that CSharpier allows to configure a max-length per line which is what we want to achieve (to be able to see code without scrolling left and right all the time). Though, dotnet-format will reorder the usings just like VS do when CTRL + K + D or CTRL + R + G which CSharpier doesn't do. Also dotnet format was a lot more slower than CSharpier when doing a bulk update. |
|
This will break every PR we have pending. It's the same reason why we have not yet converted all of our classes to use file-scope namespaces.
|
|
Also, CSharpier does not yet support c#12. Not sure if this is a problem or not for us. |
|
Yes, seems like it has issues with Primary constructors. Does'nt remove the private readonly vars properly. Also, dotnet-format on the counterpart doesn't do much line-length wrapping. |
|
I am do not mind the values used. But, I do not want to break every PR as result. But, at some point we would have to decide when to close these old PR so we are able to make this kind of change easily. |
|
you're not breaking every pr, just creating merge conflicts. resolve the merge conflict from the other pr side, and save it, and it will reformat. |
|
Maybe ... running the same tool/options only of the modified files in the PRs would fix the merge conflicts |
# Conflicts: # src/OrchardCore.Modules/OrchardCore.Cors/Controllers/AdminController.cs # src/OrchardCore.Modules/OrchardCore.Deployment.Remote/Controllers/ExportRemoteInstanceController.cs # src/OrchardCore.Modules/OrchardCore.Media/Recipes/MediaStep.cs # src/OrchardCore.Modules/OrchardCore.Workflows/Http/Activities/HttpRequestTask.cs
|
This pull request has merge conflicts. Please resolve those before requesting a review. |
|
Is there an option to remove the trailing comas when multi-lines is converted to single lines? |
|
No, there is only few configuration options. |
|
I guess we would need to combine dotnet-format and CSharpier to get to a complete result with all these options. |
|
yeah the point with |
| @@ -0,0 +1,6 @@ | |||
| { | |||
| "printWidth": 180, | |||
There was a problem hiding this comment.
While opinions on the values of this vary greatly and I couldn't find a scientific answer, 180 seems higher than what everyone recommends. After much deliberation we at Lombiq settled with 120 characters like a decade ago, and it has been working well since, across generations of screen sizes (since it's rather dependent on human anatomy).
|
I like this, although I got PTSD from the amount of files changed just by getting a glimpse of the number in the top-right tab :D. But we need it automated, otherwise, we'll need to redo it manually all the time. So, either reformat automatically on push from GitHub Actions as others mentioned, or run it with |
|
It seems that this pull request didn't really move for quite a while. Is this something you'd like to revisit any time soon or should we close? Please comment if you'd like to pick it up. |
|
Closing this pull request because it has been stale for very long. If you think this is still relevant, feel free to reopen it. |
Ran
dotnet csharpier .on the root folder of Orchard Core to reformat everything.Adds Csharpier configs and VS Code settings for CSharpier.
TODO: Make it a Github Action
See example here: https://github.com/dotnet/arcade/blob/main/.github/workflows/backport-base.yml