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.

Sign up for GitHub

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

Jump to bottom

Implement Fuzzy Search #174

Merged
merged 20 commits into from Nov 7, 2019
Merged

Implement Fuzzy Search #174

merged 20 commits into from Nov 7, 2019

Conversation

iskandarzulkarnaien
Copy link

@iskandarzulkarnaien iskandarzulkarnaien commented Oct 31, 2019 •
edited

Implement Fuzzy Search. Only merge after PR #181 is merged PR #181 is merged.

@iskandarzulkarnaien iskandarzulkarnaien added this to the v1.4 milestone Oct 31, 2019
@iskandarzulkarnaien iskandarzulkarnaien self-assigned this Oct 31, 2019
@coveralls
Copy link

coveralls commented Nov 1, 2019 •
edited

Coverage Status

Coverage decreased (-1.6%) to 51.625% when pulling a258753 on fuzzy-search into 2d3daee on master.

@iskandarzulkarnaien iskandarzulkarnaien marked this pull request as ready for review November 1, 2019 07:04
@iskandarzulkarnaien iskandarzulkarnaien changed the title Implement Fuzzy Search [MERGE AFTER #181] Implement Fuzzy Search Nov 1, 2019
@iskandarzulkarnaien
Copy link
Author

Please review my code first. Right now fuzzy find is its own command called using fuzfind but ill change that soon

@iskandarzulkarnaien iskandarzulkarnaien changed the title [MERGE AFTER #181] Implement Fuzzy Search Implement Fuzzy Search Nov 2, 2019
@iskandarzulkarnaien iskandarzulkarnaien mentioned this pull request Nov 2, 2019
Closed
@WilliamRyank
Copy link

Do we need 2 Find Command Class? or make normal find fuzzy? whats the idea of separating normal find and fuzzy

@iskandarzulkarnaien
Copy link
Author

Do we need 2 Find Command Class? or make normal find fuzzy? whats the idea of separating normal find and fuzzy

The idea is that find is fuzzy while exactFind reduces clutter by only displaying exact matches. I'll change the class names to reflect this

C-likethis123
C-likethis123 requested changes Nov 6, 2019
View reviewed changes
Copy link

@C-likethis123 C-likethis123 left a comment

Choose a reason for hiding this comment

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

Good job for this PR! I've suggested a change.

C-likethis123
C-likethis123 approved these changes Nov 6, 2019
View reviewed changes
Copy link

@C-likethis123 C-likethis123 left a comment

Choose a reason for hiding this comment

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

LGTM. As discussed earlier, type validation should be done in another PR. Good job!

WilliamRyank
WilliamRyank approved these changes Nov 6, 2019
View reviewed changes
Copy link

@WilliamRyank WilliamRyank left a comment

Choose a reason for hiding this comment

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

LGTM do check some of my comments

@nus-se-bot nus-se-bot mentioned this pull request Nov 6, 2019
Open
17 tasks
@iskandarzulkarnaien iskandarzulkarnaien merged commit 47238ec into master Nov 7, 2019
@nus-se-bot nus-se-bot mentioned this pull request Nov 14, 2019
Open
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@C-likethis123 C-likethis123 C-likethis123 approved these changes

@WilliamRyank WilliamRyank WilliamRyank approved these changes

Assignees

@iskandarzulkarnaien iskandarzulkarnaien

Labels
None yet
Projects
v1.4
  
Awaiting triage
Milestone
v1.4
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@iskandarzulkarnaien @coveralls @WilliamRyank @C-likethis123