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

Enforce code style rule IDE0055 #21199

Merged
merged 1 commit into from Nov 16, 2023
Merged

Conversation

RoosterDragon
Copy link
Member

Enforce IDE0055 Formatting rule https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/style-rules/ide0055

This rule no longer appears to be buggy, so enforce it. Some of the automated fixes are adjusted in order to improve the result. #pragma directives have no option to control indentation, so remove them where possible.

The default options are generally workable for us. We have a mix of existing styles for csharp_indent_case_contents_when_block, but using the non-default results in a smaller diff.

pchote
pchote previously approved these changes Nov 11, 2023
This rule no longer appears to be buggy, so enforce it. Some of the automated fixes are adjusted in order to improve the result. #pragma directives have no option to control indentation, so remove them where possible.
@@ -37,7 +37,8 @@ public class ModelRenderable : IPalettedRenderable, IModifyableRenderable
: this(renderer, models, pos, zOffset, camera, scale,
lightSource, lightAmbientColor, lightDiffuseColor,
color, normals, shadow, 1f,
float3.Ones, TintModifiers.None) { }
float3.Ones, TintModifiers.None)
{ }
Copy link
Member

Choose a reason for hiding this comment

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

This one I am not a big fan of. Is there anything we can do?

Copy link
Member

Choose a reason for hiding this comment

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

having constructors be forced to have newline braces doesn't bother me

Copy link
Member Author

Choose a reason for hiding this comment

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

The option csharp_preserve_single_line_blocks = true is allowing items that are wholly one one line such as Method(int a) { } to be preserved.

But as the parameters are already wrapped here, the csharp_new_line_before_open_brace = all option is jumping in to say the method body must be wrapped onto a new line.

There aren't any options to treat this differently that I can see. If you're not a fan of the style change, maybe consider if it is "acceptable losses" for enforcing the rule as a whole.

@@ -196,18 +196,24 @@ public void SetPosition(Actor self, WPos pos)
}

public Activity MoveTo(CPos cell, int nearEnough = 0, Actor ignoreActor = null,
bool evaluateNearestMovableCell = false, Color? targetLineColor = null) { return null; }
bool evaluateNearestMovableCell = false, Color? targetLineColor = null)
{ return null; }
Copy link
Member

Choose a reason for hiding this comment

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

Same here. Looked better before, although they should probably become lambdas already.

Copy link
Member Author

Choose a reason for hiding this comment

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

Same issue as above.

Copy link
Member

Choose a reason for hiding this comment

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

these braces should be unwrapped into 3 lines. Thought again this is TDGunboat, a hack trait already

Copy link
Member

@penev92 penev92 left a comment

Choose a reason for hiding this comment

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

I am not a big fan of some aspects of this rule, but I'll take it for the greater good, thank you 👍

@penev92 penev92 merged commit 360f24f into OpenRA:bleed Nov 16, 2023
3 checks passed
@penev92
Copy link
Member

penev92 commented Nov 16, 2023

Changelog

@RoosterDragon RoosterDragon deleted the style-ide0055 branch November 16, 2023 16:06
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.

None yet

4 participants