-
-
Notifications
You must be signed in to change notification settings - Fork 20
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
Fix caps, keyboard group logic #39
Fix caps, keyboard group logic #39
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change has the effect that only upper-case replacements are suggested to replace upper-case characters, and only lower-case replacements are suggested to replace lower-case characters.
@@ -77,6 +77,8 @@ function suggest(value) { | |||
|
|||
while (++index < length) { | |||
character = value.charAt(index) | |||
before = value.slice(0, index) | |||
after = value.slice(index + 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we slice
the word at the index of the character
in the input word. From my perspective, this more accurately simulates keyboard errors than the current approach.
@@ -90,8 +92,6 @@ function suggest(value) { | |||
continue | |||
} | |||
|
|||
before = value.slice(0, position) | |||
after = value.slice(position + 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does the current approach slice
the word at the position of the character
in an unrelated group
string such as the keyboard group "qwertzuop"?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Nice! I think the new suggestions ( |
3a26271
to
bb64a97
Compare
Update: The PR on my own fork has since been merged. I've reworked this PR to be more efficient, differ less from To frame the decision that remains: I have opened a PR against this PR on my own fork. It should be very quick to look at the 4 lines that PR changes in order to get a sense of why I believe that PR makes more logical sense, but will cause more breaking changes for anyone who relies on the exact ordering of the results. Practical resultsI've compared this PR against the PR on my own fork using a small test suite of single-letter errors in all 5-letter English words starting with T and ending with e. You can see the comparison here with this PR on the left and the PR on my own fork on the right. In general, the PR on my own fork tends to favor shorter suggestions (made by removals) unless there are more specific suggestions that are actually based on keyboard groups instead of more or less at random. Here are some examples of how the top suggestions differ:
Odd observationFor all the words I've tested, this PR's top suggestion always ends in "e". That's explained by the fact that the fifth letter in each word will take replacements based on the "t" in "qwertzuop", which includes an "e". It's notable that "e" is the most common letter, but this benefit only befalls the fifth letter in words containing a t! In essence, without the two changes on my own fork, the status quo in words with a "t" is to always prefer an "e" for the fifth letter... because "T" is the fifth key on the keyboard. It's madness, but it happens to work okay. |
Nice, thanks for all the research! Your extra changes on the PR to the PR also look good. I do wonder how much they’d change the tests: from your explanation in the comment above I assume they’d make more sense, in which case, I’m 👍 on it |
160efa2
to
f16a3ee
Compare
f4469c2
to
13ba741
Compare
81c0f1b
to
7d6a8de
Compare
7d6a8de
to
830d803
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There are essentially two new options for how the "Ghandi" case should be handled:
For the difference between the two options, see issue #46. |
I'd just like to clarify that this PR is ready to merge. |
This comment has been minimized.
This comment has been minimized.
I realize the new issue #46 is off-topic for this PR, so I've reverted changes that address that issue. To make this PR easier to follow, I've collapsed any off-topic/outdated comments I've made. This PR now only addresses issues #37 and #41, as originally intended. Now it's easiest to understand this PR by looking at the files changed. Despite all the commits I've added to this PR, it only alters lib/suggest by +11/-2 lines and test/index by +13/-7 lines. |
Awesome, thanks! |
released in 2.1.5! |
Update: For the latest status, see my latest comment.
This closes issue #37, closes issue #41, and ignores issue #46.
This addresses a
retext-spell
issue I've discovered that is fundamentally annspell
issue.Addressing a bigger problem
In
main
, the notion of "Keyboard groups" doesn't really make sense as it replaces at a position (of a character in group) that is unrelated to the original position (of a character in word). This solution attempts to address that "root problem."For another solution that ignores issue #41 but solves issue #37, see my alternative PR.This solution splits the word at the original index of the character in the input word instead of the unrelated position of the character in the keyboard group. See my review comments.
It's ready to merge!