-
Notifications
You must be signed in to change notification settings - Fork 589
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
refactor: delete custom entities #3262
Conversation
5dae850
to
8690132
Compare
I guess we never filled out that bit of functionality in v2. we did process them in the v1 PerformUpdate call: This was supposed to work in v2 also, but we forgot to add it. There seems to be little use of it in practice given that there are no user reports of it breaking. We may consider removing it as such, which would make custom entities only usable in DB-backed mode (which does not require this, since you can instead add the custom entities directly, without KIC, although they won't be KIC-managed). I don't know if we have stats on how often we see custom plugins with custom DAOs in the wild. Anecdotally they're fairly uncommon. |
8690132
to
b458e9a
Compare
Codecov ReportBase: 74.0% // Head: 74.1% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #3262 +/- ##
=======================================
+ Coverage 74.0% 74.1% +0.1%
=======================================
Files 110 110
Lines 13268 13227 -41
=======================================
- Hits 9826 9811 -15
+ Misses 2811 2792 -19
+ Partials 631 624 -7
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
b458e9a
to
22dd7bd
Compare
What this PR does / why we need it:
It seems that custom entities where never used (at least in a handful of recent versions).
The notion of them was "somehow" introduced in #1339 and #2296 made
sendconfig.PerformUpdate()
always passnil
in place for "custom entities".Since it's always
nil
we may as well just delete it altogether.Special notes for your reviewer:
There is still the CLI flag
I'd argue that we can add a deprecation notice and state that it doesn't do anything now and that it will be removed in next major release. WDYT?This PR marked the flag as deprecated with
flagSet.MarkDeprecated
.