Skip to content
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

Add TrimmingStringNormalizer #35

Merged
merged 1 commit into from Apr 2, 2018
Merged

Add TrimmingStringNormalizer #35

merged 1 commit into from Apr 2, 2018

Conversation

thiemowmde
Copy link
Contributor

@thiemowmde thiemowmde commented Aug 13, 2015

Pinging @brightbyte.

This is a first step. It's possible to either add all regular expressions from \Wikibase\StringNormalizer to this class or to a new, similar class.

Bug: T47925

@brightbyte
Copy link

I'm confused why we would need this. We already have a StringNormalizer that does trimming and utf8 normalization. It lives in Wikibase.

We just need ValueView to actually start using it.

@thiemowmde
Copy link
Contributor Author

You actually listed the reason why I did this: The normalizer we currently use does have an unwanted dependency on MediaWiki.

* Most simple implementation of a StringNormalizer that does nothing but trimming whitespace, by
* using the definition of "whitespace" in Unicode-enabled Perl regular expressions.
*
* @since 0.3
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0.3.2

@Ladsgroup
Copy link
Contributor

Do we still need this? Last update on it was two years ago!

@thiemowmde
Copy link
Contributor Author

I still think this is a useful addition. We can at least use this in tests, and possibly simplify some of the more specific implementations in Wikibase that depend on MediaWiki.

@Ladsgroup
Copy link
Contributor

Okay, So I assigned this to you and added the phab card to the sprint board to make sure this gets done (in less than two years hopefully) https://phabricator.wikimedia.org/T47925

Copy link
Member

@JeroenDeDauw JeroenDeDauw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a bugfix, so should increment the minor part of the version number as per http://semver.org/

Copy link
Member

@JeroenDeDauw JeroenDeDauw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nvm my previous comment, I forgot this lib is stupidly still in the 0.x range.

@thiemowmde
Copy link
Contributor Author

this lib is stupidly still in the 0.x range.

Do you realize you just called peoples decisions "stupid"?

@mariushoch
Copy link
Member

What's up with this?

@thiemowmde
Copy link
Contributor Author

I uploaded this 2015, trying to demonstrate the most simple, carefully designed alternative for parts of the badly designed \Wikibase\StringNormalizer utility class. I rebased this patch again now, but to be honest I'm tired of arguing why getting rid of \Wikibase\StringNormalizer and it's unnecessary MediaWiki dependency would be a good move.

@mariushoch
Copy link
Member

Hm, it seems that StringNormalizer no longer depends on MediaWiki. Do you think it would make sense to move that?

@thiemowmde
Copy link
Contributor Author

I still think a trivial trimming normalizer as proposed in this patch is helpful. But yea, the MediaWiki code used in Wikibase's StringNormalizer is now in a separate component called wikimedia/utfnormal. This means we actually can move it out of Wikibase now.

@mariushoch mariushoch merged commit d915e78 into master Apr 2, 2018
@mariushoch mariushoch deleted the trimNormalizer branch April 2, 2018 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants