Make StringFormatter trim value #3
Conversation
| @@ -30,7 +30,7 @@ public function format( $dataValue ) { | |||
| throw new InvalidArgumentException( 'DataValue is not a StringValue.' ); | |||
| } | |||
|
|
|||
| return $dataValue->getValue(); | |||
| return trim( $dataValue->getValue() ); | |||
There was a problem hiding this comment.
Edit: Can't be null, no check needed.
|
-2. Absolutely not. You cannot simply change the contract of a class used by other components like this. I'm also somewhat dubious on doing trimming like this in a formatter - though am not going to object against strongly against having one that does that in Wikibase. |
|
I think this is a misunderstanding. Compare to the other ValueFormatterBase based classes. They return formatted dates and such. What do you expect from a formated generic string? Trimming sounds very reasonable to me. If this class does nothing what's the reason to have it in the first place? One thing confuses me, though: The format() calls in other ValueFormatterBase based classes return HTML. Shouldn't this return htmlspecialchars( trim( ... ) )? |
|
It is a misunderstanding yes, though not from my side.
Indeed.
This class is essentially an adepter.
It is, and creating a formatter that does it is reasonable as well. The problem with this change is that it changes the default behaviour of the class. That change makes sense for the use we currently make of this class in our project. It does not automatically make sense for all users though. (And we are already no longer the only user of this code.) If this went in like this, you'd have a breaking change which is not documented, which is quite subtle and can cause data loss in applications making use of this component. One thus needs to avoid changing the default behaviour, which can be done by creating a new class or by adding options. (The former option probably being the better.) |
|
I am also curious: why are we putting trimming in a formatter to begin with? If we have a string in our db with a space, then why would we not want to show it? It seems to me that if we would not want to show it, we'd not want to have it in the db to begin with. |
|
These are the things I raised when wikimedia/data-values-value-view#20 was initially proposed. |
|
Ok, closing again since it looks like both implementation and the concept are flawed. Only reopen if you provide a reason why the concept is not flawed, and intend to fix the implementation issues |
No description provided.