Skip to content

Fixes #2156 - BREAKING CHANGE - Fixes IsOverridden and makes it internal#2158

Merged
tig merged 11 commits intogui-cs:developfrom
tig:fixes_isoverridden
Nov 2, 2022
Merged

Fixes #2156 - BREAKING CHANGE - Fixes IsOverridden and makes it internal#2158
tig merged 11 commits intogui-cs:developfrom
tig:fixes_isoverridden

Conversation

@tig
Copy link
Copy Markdown
Member

@tig tig commented Nov 2, 2022

Fixes #2156

BREAKING CHANGE -

This PR

  • Fixes IsOverridden(), which actually didn't work
  • Moves IsOverridden to Responder making it both static and internal.
  • Adds unit tests proving it works correctly now

I used cs.github.com to gain confidence that no-one outside of core code uses this function.

If someone wants to use it, they can code it up themselves... It is very bad OO practice to do something like this and we should not encourage it. In v2 we should figure out how to restructure things so it can be completely removed.

@tig tig added the breaking-change For PRs that introduces a breaking change (behavior or API) label Nov 2, 2022
@tig tig requested a review from migueldeicaza as a code owner November 2, 2022 22:24
@tig
Copy link
Copy Markdown
Member Author

tig commented Nov 2, 2022

Since other PRs are dependent on this, I'm merging it now. We can back it out if someone has problems with how I did it.

@tig tig merged commit 1ad5590 into gui-cs:develop Nov 2, 2022
This was referenced Nov 4, 2022
@tig tig deleted the fixes_isoverridden branch April 3, 2024 01:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking-change For PRs that introduces a breaking change (behavior or API)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

View.IsOverridden doesn't work

2 participants