QueryProfilerDecorator to replace QueryData #17
Conversation
ghost
assigned 
|
Is this a resubmit of something on Gerrit? |
|
This only exists on the github.com/SemanticMediaWiki/SemanticMediaWiki repo (I wanted to see how a larger change is handled in github.com/Travis without Gerrit). |
|
In that case, can you provide a brief high level description of this change? |
SummaryThe change does the exact same thing as [1](this is also why the #ask, #show tests didn't need any alteration besides the constructor) but it injects a Subobject instead of a Title object (which makes it independent from a MW data object) but more importantly it now uses a Decorator [2] for construction meaning that an additional profiling item such as duration would add a new component instead of changing the content of the original class and with it leaving existing tests untouched or unharmed. Splitting the class lowers the SRP violation from [1] due to the elimination of multi-object injection that persisted before making it possible for a decorator component to handle only a specific object (QueryFormat, QueryDescription etc.). Maybe createProfileEntry() is a better name than addProfilingData() for the QueryProfiler interface. Follow-up
[1] https://github.com/SemanticMediaWiki/SemanticMediaWiki/blob/master/includes/query/QueryData.php [2] new NullProfiler( ... ), new DescriptionProfiler( ... ), new FormatProfiler( ... ) PS: I somewhat like the review in Gerrit better, the review process in Github seems a bit off especially for larger changes. |
| * | ||
| * @return string|null | ||
| */ | ||
| protected function disabled() { |
There was a problem hiding this comment.
Though unchanged in this commit, this is a bad method name.
|
I don't see any real issues with this, feel free to merge |
## Summary The change does the exact same thing as [1] (this is also why the #ask, #show tests didn't need any alteration besides the constructor) but it injects a Subobject instead of a Title object (which makes it independent from a MW data object) but more importantly it now uses a Decorator [2] for construction meaning that an additional profiling item such as duration would add a new component instead of changing the content of the original class and with it leaving existing tests untouched. Splitting the class lowers the SRP violation from [1] due to the elimination of multi-object injection that persisted before making it possible for a decorator component to handle only a specific object (QueryFormat, QueryDescription etc.). ## Follow-up * Move classes into SMW\Query\Profiler namespace
QueryProfilerDecorator to replace QueryData
Should probably be moved into SMW\Query\Profiler ns but this something tha can be done in a follow-up after Travis