Korean future fix #40
Conversation
src/korean.js
Outdated
| if(input.length === 3) { | ||
| unicodeTotal += input[2]; | ||
| } | ||
| const finalWord = String.fromCharCode(unicodeTotal); |
There was a problem hiding this comment.
can we simply return String.fromCharCode(unicodeTotal) without assigning?
There was a problem hiding this comment.
@minsooshin you're right. I'll fix that right away.
|
@cecilebertin I don't know the history of this, but I think there is an issue on future tense. Could you take a look at #41? |
test/korean_tests.js
Outdated
| let presentWord = kc.conjugate('하다', {tense: 'future'}); | ||
| expect(presentWord).to.equal('할꺼야'); | ||
| let futureWord = kc.conjugate('하다', {tense: 'future'}); | ||
| expect(futureWord).to.equal('할꺼야'); |
There was a problem hiding this comment.
I think it should be 할 거야. 할꺼야 is pronunciation.
There was a problem hiding this comment.
@minsooshin Thanks! I'll add a fix for this too.
There was a problem hiding this comment.
@minsooshin I looked it up, http://www.koreanwikiproject.com/wiki/A/V_%2B_(으)ㄹ_거예요
I think there should be a space in between for all the cases.
There was a problem hiding this comment.
@cecilebertin that's right. Also, the example you provide is to senior people (Informal polite). So, for your test cases, I think To make the low form (반말), the 예요 part becomes 야: A/V+(으)ㄹ 거야 is the correct reference. That's what I said before; it should be 할 거야. There is a whitespace between 할 and 거야.
|
@minsooshin , yes I just saw that. I'll take a look at it this afternoon with @yjlim5 |
|
@cecilebertin thanks!!! |
src/korean.js
Outdated
| stem.push(8); | ||
| return `${combineSymbols(stem)} 꺼야`; |
There was a problem hiding this comment.
return `${combineSymbols(stem)} 거야`;not 꺼야
There was a problem hiding this comment.
꺼야 is written the pronunciation.
There was a problem hiding this comment.
@minsooshin I think you're right again :) ,I looked it up. Nice catch!
I'll add another commit to fix this.
There was a problem hiding this comment.
@cecilebertin thanks a lot for quick fix and feedback!!!
There was a problem hiding this comment.
@minsooshin thanks for the thorough review!!!
|
Thanks for catching the details and fixing them @cecilebertin and @minsooshin. Great team work! |
When I updated the combineSymbol function, I realized the future tense didn't work properly.
The if statement (if (stem[-1] !== 8), line 132) wasn't working correctly and I realized it was coded completely wrong. It pushed 8 to the array for all cases, and we ended up with arrays of 3 or 4 jamo's for latest character of the stem. 3 should be de maximum number of jamo's possible.
We were lucky it passed all the tests before, because the previous combineSymbol function didn't limit the length of the array.
I also realized we missed some cases for the future tense : when the stem ends with a ㅂ consonant, and the other verbs with a stem ending with a consonant other than ㄹ.