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

Fix conditional traits that incorrectly override INotifyCreated. #16770

Merged
merged 2 commits into from Jul 13, 2019

Conversation

@pchote
Copy link
Member

commented Jul 13, 2019

This also adds a check command to make sure these errors can't happen again.

@pchote pchote added this to the Next Release milestone Jul 13, 2019

@pchote pchote force-pushed the pchote:conditional-trait-check branch from 2360c2f to e0535c2 Jul 13, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor

commented Jul 13, 2019

I locally reverted only the DetectCloaked check to test the lint rule, but it returns 214(!) violations for me...

Edit: And it listed ReloadAmmoPool even though I did not revert that fix, so something is definitely wrong with the lint check.

@pchote

This comment has been minimized.

Copy link
Member Author

commented Jul 13, 2019

Can you paste some of the output it gives? It works correctly for me and travis, so this may be a windows-specific issue.

@reaperrr

This comment has been minimized.

Copy link
Contributor

commented Jul 13, 2019

error1

I guess it just says that for every trait implementing ConditionalTrait.

Add --check-conditional-trait-interface-overrides utility command.
This command is used by `make check` to detect traits that incorrectly
override interface methods that are required for conditions to work
correctly.

@pchote pchote force-pushed the pchote:conditional-trait-check branch from e0535c2 to 44f88e5 Jul 13, 2019

@pchote

This comment has been minimized.

Copy link
Member Author

commented Jul 13, 2019

It seems that the default behaviour wrt BindingFlags.DeclaredOnly is different between mono and .NET. Fixed.

@teinarss
Copy link
Contributor

left a comment

Looks good tested the check command.

@teinarss teinarss merged commit 579d2c1 into OpenRA:bleed Jul 13, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@pchote pchote deleted the pchote:conditional-trait-check branch Aug 26, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.