Drop ValueFormatterTestBase::getFormatterClass #10
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Good question. This looks like a mistake that no one noticed until now. Not sure moving one is worthwhile. To me the real issue is that they are part of the packages public interface, and that simple clean-up like you are doing here is causing breaking changes affecting multiple components. |
|
Replacing the abstract test classes with something more encapsulated would be nice. |
|
"Nice to have" can not be merged, no matter how many people agree on that. This patch is a step in that direction and can be merged. |
|
Indeed. Note how I have not provided any review opinion. All I did was answer your questions ;) |
JeroenDeDauw
added a commit
that referenced
this pull request
Mar 13, 2015
Drop ValueFormatterTestBase::getFormatterClass
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Similar to DataValues/Common#25:
Please review #9 (commit 1) first, merge it, then rebase this patch and review it separately. Or even better: Please move this component to Gerrit so I can create an actual chain that's actually reviewable?
Also: Why is this base class in Interfaces while the other is in Common?
Bug: T92268