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 Quality Rules #20812

Merged
merged 6 commits into from
Jun 20, 2023
Merged

Conversation

RoosterDragon
Copy link
Member

Following on from #20802

Enforces 5 code quality rules across the project.

I have batched together a PR for these rules as they each had some but not too many violating files.

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

OpenRA.Game/HotkeyManager.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/LoadScreens/SheetLoadScreen.cs Outdated Show resolved Hide resolved
Copy link
Member

@PunkPun PunkPun left a comment

Choose a reason for hiding this comment

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

I don't agree with CA1816. All these classes should be inheritable.

@pchote
Copy link
Member

pchote commented Apr 17, 2023

For things like traits I wouldn't necessarily disagree, but the cases here are internal engine types that don't make much sense to subclass (with the exception of Map, but that can be unsealed if/when needed).

@RoosterDragon
Copy link
Member Author

For CA1816 I don't see a good reason to allow inheritance for any of them. They've got no protected or virtual methods, they've got no behaviour a derived class could customize (except ToString, anyway). There's nothing code wanting to make use of the class couldn't already achieve via composition.

Mailaender
Mailaender previously approved these changes Jun 3, 2023
@abcdefg30 abcdefg30 merged commit 231bf01 into OpenRA:bleed Jun 20, 2023
3 checks passed
@PunkPun
Copy link
Member

PunkPun commented Jun 20, 2023

Changelog

@RoosterDragon RoosterDragon deleted the style-ca-more branch June 27, 2023 17:36
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

6 participants