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
DolphinTool: Ditch OOP design #11953
DolphinTool: Ditch OOP design #11953
Conversation
bde6900
to
c217db0
Compare
|
This was asked on IRC and you didn't really respond to it, so I'll reask it here... why? Why do you think this is better? Granted, yes, a few of these methods can and probably should be standalone, but I'm not really sure whether dismantling the Commands is really all that better (or worse, for that matter)? |
|
It's because I don't want further developments to DolphinTool to be stuck with this outdated, useless design. When I put in HeaderCommand, I just copy-pasted a bunch of boilerplate to make stuff work. When I eventually implement ExtractCommand (when these PRs finally get attention and are approved, one at a time, over the course of weeks), I don't want to copy fluff again just to implement it. I don't want my code to stick out by being better. |
|
These "commands" are basically sub-main functions that receive the arguments they are supposed to see. This OOP design does nothing to make that possible. Removing it would do no damage in the present or future. It was the wrong choice. |
|
I agree with removing the polymorphism. If we were passing the command pointer around and the notional users didn't care about which command it was it would be useful, but here it's just abstraction without any benefit and more code. |
c217db0
to
e1aa78e
Compare
|
While I don't appreciate your attitude, I do agree that it seems a bit needless in this case, so I'm fine with it being changed. |
e1aa78e
to
f5dda04
Compare
f5dda04
to
25c020c
Compare
|
Sorry, I neglected to fix the other place that HeaderCommand was forgotten to be added to the filters. |
|
It's fine, I'm not even sure why we have a |
This was never necessary and I don't see it becoming useful.
Depends on #11951 because I don't feel like untangling it just to resolve conflicts later.