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

feat(): centralize the .editorconfig to the root for consistency #739

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

joefeser
Copy link
Contributor

@joefeser joefeser commented Aug 5, 2024

Description

Enables all of the c# projects to have consistent rules and formatting.

Issues Resolved

Not having to fix tab / space inconsistencies

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

.editorconfig Outdated
@@ -3,7 +3,7 @@ root=true
[*]
charset = utf-8
end_of_line = lf
indent_style = space
indent_style = tabs
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this a settled debate where things look different on Windows vs. *nix with tabs? :)

Can we use 2 spaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could do that, but that would require changing the entire project. Every single current file is formatted with tabs, which is why I kept the setting

Copy link
Member

Choose a reason for hiding this comment

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

Got it. Leaving it to @Xtansia :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

While it's not currently fully consistent as most older files still have tabs, all newer files are indented with spaces, and generated code is opinionatedly formatted with spaces.

I'd rather go to the effort of unifying things on spaces than to revert this setting, especially given reverting this setting still doesn't fix the inconsistencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The goal would be to run a dotnet format on everything to get everything consistent. Do we want to also talk about some of the other settings that we want to unify, like no {} on namespaces or any other .editorconfig niceties?

Copy link
Member

Choose a reason for hiding this comment

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

I don't have a strong preference, deferring to @Xtansia. We'll take any imrovement!

Copy link
Contributor Author

@joefeser joefeser Aug 6, 2024

Choose a reason for hiding this comment

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

A lot of the noise is around these 3 use cases:

//space mismatch on custom attributes on one folder
[DataMember(Name ="path_match")]

[DataMember(Name = "path_match")]

//spacing for object creation
Summaries.Add(new RunSummary {Total = skipReasons.Count, Skipped = skipReasons.Count});

Summaries.Add(new RunSummary { Total = skipReasons.Count, Skipped = skipReasons.Count });   

//class devs for empty classes
public class ValueCountAggregationDescriptor<T>
  : FormattableMetricAggregationDescriptorBase<ValueCountAggregationDescriptor<T>, IValueCountAggregation, T>
    , IValueCountAggregation
  where T : class { }

public class ValueCountAggregationDescriptor<T>
  : FormattableMetricAggregationDescriptorBase<ValueCountAggregationDescriptor<T>, IValueCountAggregation, T>
    , IValueCountAggregation
  where T : class
{ }

I am still cleaning up the sample PR of what the outcome of the .editorconfig will be. I need a few hours

Copy link
Collaborator

Choose a reason for hiding this comment

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

From my perspective for ease of review-ability, this will need 3 PRs (not necessarily in this order):

  • Change ApiGenerator to invoke dotnet format rather than CSharpier to ensure styling will be consistent in generated code
  • Adjust .editorconfig(s) to be consistent and centralized etc.
  • Run dotnet format

Depending on the style change I'm not particularly fussy (other than keeping spaces), I think the change to non-braced namespaces would be fine to include especially given nearly every file is already going to have whitespace/indent changes anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR #747 Created. Adjust ApiGenerator

Copy link
Collaborator

Choose a reason for hiding this comment

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

Given dotnet format is very adverse to line breaking/wrapping/chopping based on line length, there's very little that it does to change formatting outside of some indent and spacing options. The code-style/lint analysis is very useful though. So there might be an argument to use a combination of CSharpier & dotnet format.

I'm going to do a bit of assessment of options and see what's most feasible.

Signed-off-by: Joe Feser <joseph.feser@gmail.com>
Signed-off-by: Joe Feser <joseph.feser@gmail.com>
Signed-off-by: Joe Feser <joseph.feser@gmail.com>
Signed-off-by: Joe Feser <joseph.feser@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants