Adding and optimizing normalization rules

[benfei]

New normalization rules:

• replace_charachters, replaces given characters with a replacement string.
• replace_emails

Optimized normalization rules:

• optimize replace_punctuation using str.translate(). benchmarks on my pc shows x10-x30 speedup on my laptop. It's unnoticed on short strings, but may be significant on large texts.
• skip the unecessary strip() step in remove_extra_whitespaces.

[davidmogar]

Hi @feinsteinben,

Thank you for your pull requests. Some comments on this one:

• Would be nice if you can normalize your commits messages. No need for a tag. Just accommodate them to the ones already on the project.
• Squash your commits. At least commit 45b37c5 can be fixup with 61aae57.
• [61aae57] maketrans is only available on Python 3. The goal is to make this library to work on Python 2.7. That will need a fix.
• [18839ad] On hold cause it uses replace_characters.
• [9d42b96] My god, feel so stupid seeing my old code xD This commit shouldn't change punctuation anyway :stuck_out_tongue_winking_eye:
• [179146b] Totally right. Good eye.
• [65564ac] Changes again punctuation. All changes should be on the same commit. Easy to follow later. I really like the change to _parse_normalizations.

Thanks again for you request. If you don't feel like fixing this let me know and I'll do it. All good changes.

Cheers, David

[benfei]

Hi @davidmogar ,

Thanks for your review.

I rebased the pull-request according to your comments. Is it OK?

Bests, Ben.

[davidmogar]

Hi @feinsteinben,

Capitalized please? :yum:

Still, you should squash your commits (you have two commits for optimizations and two for new rules). Ideally I would have 3 commits. Also, you are still using str.maketrans in one method, which would fail. Easy to test applying all normalizations with Python 2.6.

Thanks for your effort!

Cheers, David

[benfei]

Hi @davidmogar ,

Thanks for your comments. Just to make sure I understand correctly - one commit for removing/fixing old stuff, one for adding new normalizations and the last for optimizations?

Regarding to str.maketrans - my bad. I'll add tests for that later.

Cheers, Ben

[davidmogar]

Hi @feinsteinben,

Yeah, I think is clearer that way.

Thanks! David

[benfei]

Hi @davidmogar ,

Thanks to your review, I tested my code and found some bugs (fixed now). Additionally, the commits were squashed together (I tried to follow the style from your previous commits).

Cheers, Ben.

[davidmogar]

That's perfect @feinsteinben! Please, rebase, fix conflicts and I'll merge it ;)

[benfei]

Thanks @davidmogar!

I don't really know to do what you've asked for (it's my first real contribution in GitHub), but I want to learn. I would really appreciate some guidance here.

Thanks, Ben

[davidmogar]

Hi @feinsteinben,

Just take this gist as an example.

Cheers, David

[benfei]

Done! @davidmogar

[benfei]

Hi @davidmogar ,

After the tests were merged, I found a bug in my commits. It works fine for python3, but breaks/does wrong in python2. I fixed it in the same branch as the pull-request.

Additionally, I found a wrong test-case and fixed it too (in the other pull-request. I'll add a message there, too).

Sorry for the trouble, Ben.

[davidmogar]

The wrong test-case is fixed as I commented in the other pull-request. The patch for Python 2 have to go in a new PR ;)

Cheers, David

[benfei]

I'll do it later today.

On Mon, Oct 24, 2016, 16:51 David Moreno García notifications@github.com wrote:

The wrong test-case is fixed as I commented in the other pull-request. The patch for Python 2 have to go in a new PR ;)

Cheers, David

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/davidmogar/normalizr/pull/9#issuecomment-255746091, or mute the thread https://github.com/notifications/unsubscribe-auth/AKuWBtenUlfAjyfj4VfNaO0Dz2OZnUICks5q3LfvgaJpZM4KcTQu .