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

feat: add isGurmukhi function to check if string is Unicode Gurmukhi #141

Merged
merged 10 commits into from
May 30, 2020

Conversation

sarabveer
Copy link
Collaborator

Closes #140

@sarabveer sarabveer added Effort 0 Non-work/tracking. Impacts Most Affects a majority of end-users. Status: WIP □ Type Story Feature or requirement written from the user's perspective using non-technical language. labels May 22, 2020
@sarabveer
Copy link
Collaborator Author

Need to add tests

@Harjot1Singh
Copy link
Member

I'd suggest calling it isUnicodeGurmukhi or isGurmukhiUnicode, since isGurmukhi can be easily confused with "Is this string Gurmukhi" btw.

lib/isGurmukhi.js Outdated Show resolved Hide resolved
@sarabveer
Copy link
Collaborator Author

sarabveer commented May 22, 2020

I'd suggest calling it isUnicodeGurmukhi or isGurmukhiUnicode, since isGurmukhi can be easily confused with "Is this string Gurmukhi" btw.

ASCII is ASCII, it isn't Gurmukhi. There isn't a way to test if a string is from an ASCII Gurmukhi font, and if there was, English would also return true for such a function.

@bhajneet bhajneet removed Effort 0 Non-work/tracking. Impacts Most Affects a majority of end-users. Status: WIP □ Type Story Feature or requirement written from the user's perspective using non-technical language. labels May 22, 2020
@sarabveer sarabveer changed the title Add isGurmukhi function feat: add isGurmukhi function to check if string is Unicode Gurmukhi May 25, 2020
@coveralls
Copy link

coveralls commented May 25, 2020

Pull Request Test Coverage Report for Build 301

  • 8 of 8 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.8%) to 99.2%

Totals Coverage Status
Change from base Build 297: -0.8%
Covered Lines: 95
Relevant Lines: 95

💛 - Coveralls

@sarabveer sarabveer marked this pull request as ready for review May 25, 2020 16:32
@sarabveer sarabveer self-assigned this May 25, 2020
index.d.ts Outdated Show resolved Hide resolved
test/isGurmukhi.spec.js Show resolved Hide resolved
docs/README.md Show resolved Hide resolved
@bhajneet
Copy link
Member

Since this PR is closing #140, perhaps we can also fix toAscii function in this PR?

@sarabveer
Copy link
Collaborator Author

Since this PR is closing #140, perhaps we can also fix toAscii function in this PR?

@Harjot1Singh how would this be done?

@Harjot1Singh
Copy link
Member

Harjot1Singh commented May 30, 2020

Since this PR is closing #140, perhaps we can also fix toAscii function in this PR?

@Harjot1Singh how would this be done?

toAscii = (text) => isAscii(text) ? text : otherStuffInFunction

Something like that. I'm not sure if we'd want exhaustive enabled or not.

@bhajneet
Copy link
Member

Since this PR is closing #140, perhaps we can also fix toAscii function in this PR?

@Harjot1Singh how would this be done?

toAscii = (text) => isAscii(text) ? text : otherStuffInFunction

Something like that. I'm not sure if we'd want exhaustive enabled or not.

Wouldn't it be better to fix toAscii from not running the post rules block if it's already in ascii?

@Harjot1Singh
Copy link
Member

Since this PR is closing #140, perhaps we can also fix toAscii function in this PR?

@Harjot1Singh how would this be done?

toAscii = (text) => isAscii(text) ? text : otherStuffInFunction
Something like that. I'm not sure if we'd want exhaustive enabled or not.

Wouldn't it be better to fix toAscii from not running the post rules block if it's already in ascii?

Wouldn't this mean running some of toAscii, followed by then running isAscii? Why do extra work?

@sarabveer
Copy link
Collaborator Author

There is no isAscii it is isGurmukhi

@Harjot1Singh
Copy link
Member

There is no isAscii it is isGurmukhi

My bad, isGurmukhi instead of isAscii

@bhajneet
Copy link
Member

Wouldn't this mean running some of toAscii, followed by then running isAscii? Why do extra work?

Sorry, I misread. Using isExtendedAscii/isGurmukhi would be fine in the line you said. However since isGurmukhi's exhaustive parameter requires all characters to be in unicode to pass, I do not believe it will work for shabados/desktop. (In the sense that you are trying to convert something toAscii on each keystroke of unicode keyboard and each keystroke is supposed to be converted toAscii).

@bhajneet
Copy link
Member

bhajneet commented May 30, 2020

If you use "some" instead of "every" in isGurmukhi function then it isn't true to the name of the function. Maybe better to check "every"/exhaustive for extended ascii chars and if false then converting to ascii?

Edit: or rename func to hasGurmukhi

@sarabveer
Copy link
Collaborator Author

However since isGurmukhi's exhaustive parameter requires all characters to be in unicode to pass, I do not believe it will work for shabados/desktop.

This is why I believe and argue toAscii should not be "fixed". toAscii is doing its job, but due to the nature of ASCII, the replacements have to be done after and cannot be done before.

isGurmukhi should be used by implementations of toAscii and check before calling the toAscii function.

@bhajneet
Copy link
Member

This is why I believe and argue toAscii should not be "fixed". toAscii is doing its job, but due to the nature of ASCII, the replacements have to be done after and cannot be done before.

isGurmukhi should be used by implementations of toAscii and check before calling the toAscii function.

That would still fail my use case.

@sarabveer
Copy link
Collaborator Author

sarabveer commented May 30, 2020

This is why I believe and argue toAscii should not be "fixed". toAscii is doing its job, but due to the nature of ASCII, the replacements have to be done after and cannot be done before.
isGurmukhi should be used by implementations of toAscii and check before calling the toAscii function.

That would still fail my use case.

How so? Every keystroke can be run through isGurmukhi without exhaustive. This issue would be out of scope for gurmukhi-utils.

@bhajneet
Copy link
Member

How does this work for us if we're converting each keystroke input for search query?

image

@sarabveer
Copy link
Collaborator Author

sarabveer commented May 30, 2020

Why is this issue being raised now? The functionality you have shown is expected.

Input is supposed to be explicit, either Gurmukhi or ASCII. We agreed mixing would not be allowed.

EDIT: This is reflected in the tests as well https://github.com/ShabadOS/gurmukhi-utils/blob/isGurmukhi/test/isGurmukhi.spec.js#L13

Copy link
Member

@Harjot1Singh Harjot1Singh left a comment

Choose a reason for hiding this comment

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

Add an example to examples.js too, as well as the example section at the top of README.hbs, then good to go

test/isGurmukhi.spec.js Outdated Show resolved Hide resolved
@Harjot1Singh Harjot1Singh merged commit 32a151b into master May 30, 2020
@Harjot1Singh Harjot1Singh deleted the isGurmukhi branch May 30, 2020 21:47
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.

Find Unicode Gurmukhi instead of ASCII in Search query in Lines Model
4 participants