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

Enable more code style rules #20694

Merged
merged 21 commits into from
Feb 24, 2023
Merged

Enable more code style rules #20694

merged 21 commits into from
Feb 24, 2023

Conversation

RoosterDragon
Copy link
Member

@RoosterDragon RoosterDragon commented Feb 24, 2023

Enforces 21 19 style rules across the project, and 2 complier warnings related to XML docs.

I have batched together a PR for these rules as they each had limited existing violations (4 files or less) to resolve. However fixes are arranged per commit so we can easily bikeshed over rules as desired.

See https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/style-rules/ for explanation of each rule.

@pchote
Copy link
Member

pchote commented Feb 24, 2023

These mostly look like good cleanups, except IMO 16 and 37 which i'm not convinced are improvements.

@PunkPun
Copy link
Member

PunkPun commented Feb 24, 2023

I'm concerned about IDE0016. As far as I understand those checks are there to throw early.

Also do add punctuation to the descriptions

@RoosterDragon
Copy link
Member Author

RoosterDragon commented Feb 24, 2023

Removals in IDE0016 is my fault. Given our thorough lack of argument null checks in the solution, having just three of them seemed arbitrary so I chose to remove them rather than change. However we could keep those if desired and change to throw expressions, or disable the rule.

@PunkPun
Copy link
Member

PunkPun commented Feb 24, 2023

It might be worth going through git blame and investigatie each situation

@RoosterDragon
Copy link
Member Author

Removed 16 and 37, added missing full stops.

@penev92
Copy link
Member

penev92 commented Feb 24, 2023

FWIW 37 is somehting we do elsewhere a bunch and IMO it does make sense as a rule.

# Use coalesce expression (nullable types).
dotnet_diagnostic.IDE0030.severity = warning

# Use explicitly provided tuple name.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Use explicitly provided tuple name.
# Prefer explicitly provided tuple element name.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've been copy pasting the titles of the docs, e.g. https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/style-rules/ide0033. Don't feel like changing the approach much for one item.

Copy link
Member

Choose a reason for hiding this comment

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

Weird. I commented because that's not the title VSCode gave me 🤷‍♂️ Alright then, never mind.

@penev92
Copy link
Member

penev92 commented Feb 24, 2023

Thanks! 🎊

@penev92 penev92 merged commit 6c96405 into OpenRA:bleed Feb 24, 2023
@RoosterDragon RoosterDragon deleted the style-simple branch February 24, 2023 20:06
@penev92
Copy link
Member

penev92 commented Feb 28, 2023

Changelog

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