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

Fixing issue 9 #14

Closed
wants to merge 2 commits into from
Closed

Fixing issue 9 #14

wants to merge 2 commits into from

Conversation

notalfredo
Copy link
Member

Fixes #9

I had a different approach let me know what you think. For levenshtein distance I converted the matrix computations functions as a helper function. Then diff and distance call this helper function. This way we don't have any dupe code. Hamming works similar

@neoncitylights
Copy link
Contributor

neoncitylights commented Dec 31, 2022

Thanks for the PR! Just a few comments.

If you look at the files tab, it shows the branch is outdated because it also shows splitting out the lib.rs into 4 different files (also see screenshot). Unfortunately it's not possible through GitHub itself to make a PR dependent on another PR, so this will have to wait until #13 is gone through, since it'll be a little difficult reviewing this change.

image


Can you make the commit message title a little more specific? While it technically explain what's being fixed, it's not possible to understand what's going on purely from the title without visiting a link.

To illustrate, if the commit history was like:

 - fix: Fix issue #9 
 - feat: Implement feature #120
 - docs: Fix docs #19

It'd be a bit difficult to track things down, or understand how the software was changing over time, like e.g if there was a regression (a bug was fixed at one point, but then accidentally re-introduced in the future). Since #9 says there's duplicate code, the title should be something along the lines of like:

chore: Deduplicate code between diff() and distance()

<commit message body explains how code was deduplicated>

Fixes #9

I had a different approach let me know what you think. For levenshtein distance I converted the matrix computations functions as a helper function. Then diff and distance call this helper function. This way we dont have any dupe code. Hamming works similar
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.

Duplicate code between diff() and distance()
2 participants