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

refactor: FindMatches class #1222

Merged
merged 5 commits into from
Dec 21, 2024

Conversation

miurahr
Copy link
Member

@miurahr miurahr commented Dec 18, 2024

split #963

Pull request type

  • Other (describe below)
    refactoring

Which ticket is resolved?

  • BUGS#1251

What does this PR change?

  • Add regression test of BUGS#1251
  • Refactor FindMatches class
  • Ignore the regression test

Other information

@miurahr miurahr changed the title Topic/miurahr/matches/refactor find matches class refactor: FindMatches class Dec 18, 2024
- Add FindMatchesThreadTest to reproduce BUGS#1251
- Add a test case to test FindMatches with the case of BUGS#1251.

Signed-off-by: Hiroshi Miura <[email protected]>
- add internal search method to handle normal and segmented search conditions, also use for test purpose
- drop threshold arguments for CalcMatchStatistics usage
- FindMatchesThread.finderSearch to take threshold argument for testing.
- update search() callers accordingly.

Signed-off-by: Hiroshi Miura <[email protected]>
@miurahr miurahr force-pushed the topic/miurahr/matches/refactor-find-matches-class branch from 8779852 to 2c44aed Compare December 18, 2024 04:11

This comment was marked as outdated.

Copy link

❌ Quality checks failed.

Please look a Gradle Scan page for details:
https://gradle.com/s/mllkm5smjhu6e

It should be enabled when the fix is proposed.

Signed-off-by: Hiroshi Miura <[email protected]>
@miurahr miurahr force-pushed the topic/miurahr/matches/refactor-find-matches-class branch from 62fbbc7 to f2de1ee Compare December 18, 2024 04:50
@miurahr miurahr marked this pull request as ready for review December 18, 2024 04:51
Copy link

❌ Quality checks failed.

Please look a Gradle Scan page for details:
https://gradle.com/s/3ntyp6ijmtre4

Copy link

❌ Quality checks failed.

Please look a Gradle Scan page for details:
https://gradle.com/s/wbrkcoblmka2k

- CalcMatchStatisticsTest checks with disabling threshold, so set it -1

Signed-off-by: Hiroshi Miura <[email protected]>
@miurahr
Copy link
Member Author

miurahr commented Dec 19, 2024

@t-cordonnier Please be aware of the refactoring of the FindMatches class and additions of unit tests.

@miurahr
Copy link
Member Author

miurahr commented Dec 19, 2024

A core idea of refactoring FindMatches is to give a boolean flag to the search method to separate between an ordinary call and an internal recursive call for the segment matches.

  List<NearString> search(String searchText, boolean fillSimilarityData, IStopped stop,
                            boolean runSeparateSegmentMatch) throws StoppedException {

@t-cordonnier
Copy link
Contributor

@t-cordonnier Please be aware of the refactoring of the FindMatches class and additions of unit tests.

I will have a look but in any case this branch is rebased on the very last master, so if there were conflicts I should have seen them.

@miurahr miurahr merged commit ffc7616 into master Dec 21, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants