Bug 17302 - Add Koha::Util::Normalize for normalization functions
Summary: Add Koha::Util::Normalize for normalization functions
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 17318
  Show dependency treegraph
 
Reported: 2016-09-14 17:47 UTC by Tomás Cohen Arazi
Modified: 2017-12-07 22:16 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 17302: Add Koha::Util::Normalize for normalization functions (4.98 KB, patch)
2016-09-14 17:53 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17302: Add Koha::Util::Normalize for normalization functions (5.22 KB, patch)
2016-09-15 18:29 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17302: Add Koha::Util::Normalize for normalization functions (5.32 KB, patch)
2016-09-16 07:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17302: [Follow-up] Make Normalize.pm undef-resistent (2.64 KB, patch)
2016-09-16 07:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[PASSED QA] Bug 17302: Add Koha::Util::Normalize for normalization functions (5.35 KB, patch)
2016-09-18 10:59 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 17302: [Follow-up] Make Normalize.pm undef-resistent (2.67 KB, patch)
2016-09-18 11:00 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2016-09-14 17:47:05 UTC
It'd be handy to have one place to look for string normalization routines.
Comment 1 Tomás Cohen Arazi 2016-09-14 17:53:33 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2016-09-14 19:51:24 UTC
Exporter ?
Comment 3 David Cook 2016-09-14 23:39:27 UTC
I still think that the normalization done by C4::Matcher::_normalize is a bad idea to use as a default.

We shouldn't be pre-normalizing data before it hits Zebra, because Zebra is going to try to normalize it again for searching using the same method it used to normalize data for indexing.
Comment 4 Tomás Cohen Arazi 2016-09-15 01:25:33 UTC
(In reply to David Cook from comment #3)
> I still think that the normalization done by C4::Matcher::_normalize is a
> bad idea to use as a default.

Yeah, but first things first, if we want this on master. Look at bug 17304, which is part of this effort to make normalization configurable.
Comment 5 Tomás Cohen Arazi 2016-09-15 18:29:22 UTC
Created attachment 55591 [details] [review]
Bug 17302: Add Koha::Util::Normalize for normalization functions

This patch introduces Koha::Util::Normalize, which includes the following normalization routines
that need no explanation:

- remove_spaces
- upper_case
- lower_case

and it also includes:

- legacy_default: which basically does what C4::Matcher::_normalize does.

All routines functionality are fully tested with the included in the included tests.

To test:
- Apply the patch
- Run:
  $ prove t/Koha/Util/Normalize.t
=> SUCCESS: All tests pass
- Sign off :-D

Edit: Added Exporter to explicitly export the routines.
Comment 6 Marcel de Rooy 2016-09-16 07:43:32 UTC
Created attachment 55600 [details] [review]
Bug 17302: Add Koha::Util::Normalize for normalization functions

This patch introduces Koha::Util::Normalize, which includes the following normalization routines
that need no explanation:

- remove_spaces
- upper_case
- lower_case

and it also includes:

- legacy_default: which basically does what C4::Matcher::_normalize does.

All routines functionality are fully tested with the included in the included tests.

To test:
- Apply the patch
- Run:
  $ prove t/Koha/Util/Normalize.t
=> SUCCESS: All tests pass
- Sign off :-D

Edit: Added Exporter to explicitly export the routines.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2016-09-16 07:43:36 UTC
Created attachment 55601 [details] [review]
Bug 17302: [Follow-up] Make Normalize.pm undef-resistent

Trivial changes for passing undef to the norm routines.
Added a few dumb tests too.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Katrin Fischer 2016-09-18 10:59:22 UTC
Created attachment 55649 [details] [review]
[PASSED QA] Bug 17302: Add Koha::Util::Normalize for normalization functions

This patch introduces Koha::Util::Normalize, which includes the following normalization routines
that need no explanation:

- remove_spaces
- upper_case
- lower_case

and it also includes:

- legacy_default: which basically does what C4::Matcher::_normalize does.

All routines functionality are fully tested with the included in the included tests.

To test:
- Apply the patch
- Run:
  $ prove t/Koha/Util/Normalize.t
=> SUCCESS: All tests pass
- Sign off :-D

Edit: Added Exporter to explicitly export the routines.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Katrin Fischer  <katrin.fischer@bsz-bw.de>
Comment 9 Katrin Fischer 2016-09-18 11:00:12 UTC
Created attachment 55650 [details] [review]
[PASSED QA] Bug 17302: [Follow-up] Make Normalize.pm undef-resistent

Trivial changes for passing undef to the norm routines.
Added a few dumb tests too.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Katrin Fischer  <katrin.fischer@bsz-bw.de>
Comment 10 David Cook 2016-09-18 23:23:53 UTC
(In reply to Tomás Cohen Arazi from comment #4)
> Yeah, but first things first, if we want this on master. Look at bug 17304,
> which is part of this effort to make normalization configurable.

I'm not sure how bug #17304 relates to making normalization configurable. Isn't that just adding unit tests?

I'm not convinced that there should be any normalization - configurable or hard-coded - either. If we want to normalize data for matching, we should probably be looking at Zebra's normalization rules. On that note, I'd be curious to see how ElasticSearch handles normalization. I looked a bit at how Solr did it a while ago and I recall it leaving a lot to be desired...
Comment 11 Kyle M Hall 2016-09-27 11:29:20 UTC
Pushed to master for 16.11, thanks Marcel, Tomas!