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

[JA] Add support for Hiragana #511

Merged
merged 5 commits into from
Apr 14, 2023

Conversation

yuichkun
Copy link
Contributor

TL;DR

This PR adds support for Hiragana(ひらがな) in the Japanese date parser.
I didn't know the best way of treating Hiragana/Kanji, so for now I just did it in the most straight forward, or redundant way.
If you have any thoughts on how to improve this, I'd love to challenge myself!

In addition to adding support for Hiragana, I also added some test cases that seemed to be missing.

Btw, thank you so much for writing this library, and I really appreciate how well this library is maintained.

testSingleCase(chrono.ja, "今日感じたことを忘れずに", new Date(2012, 8 - 1, 10, 12), (result) => {
expect(result.index).toBe(0);
expect(result.text).toBe("今日");
function testToday(pattern: string) {
Copy link
Owner

@wanasit wanasit Apr 9, 2023

Choose a reason for hiding this comment

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

Could you not use a function like this and repeat testSingleCase() for each case?

I know it's a lot of duplicated code, but we want to ensure the tests are really straightforward. (e.g. if the expect fails it would be more difficult to see where we break the test).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we want to ensure the tests are really straightforward

I absolutely agree with you.
I'll make the tests like so and maybe with less assertions like you mentioned here

expect(result.start.get("year")).toBe(2012);
expect(result.start.get("month")).toBe(8);
expect(result.start.get("day")).toBe(10);
expect(result.start).toBeDate(new Date(2012, 8 - 1, 10, 12));
Copy link
Owner

Choose a reason for hiding this comment

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

I think, previously, we test too many things. e.g. only expect(result.start).toBeDate() and maybe expect(result.text) is enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do so accordingly 👍

const PATTERN = /今日|当日|昨日|明日|今夜|今夕|今晩|今朝/i;
const PATTERN = /今日|きょう|当日|とうじつ|昨日|きのう|明日|あした|今夜|こんや|今夕|こんゆう|今晩|こんばん|今朝|けさ/i;

function normalizeTextToKanji(str: string) {
Copy link
Owner

Choose a reason for hiding this comment

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

Nit: text not str, because your function name is normalizeTextToKanji, not normalizeStrToKanji.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree!

@wanasit
Copy link
Owner

wanasit commented Apr 9, 2023

Hello @yuichkun. Thank you so much for the change.

Could you please take a look at my comments, especially about the test?
It would be nice if you can address them.

@yuichkun
Copy link
Contributor Author

yuichkun commented Apr 9, 2023

Hi @wanasit ,
thank you for your feedback, and I think I've fixed the points you mentioned.
Please let me know if there's anything else I should reconsider!

@yuichkun yuichkun requested a review from wanasit April 9, 2023 08:12
@wanasit wanasit merged commit bd838bd into wanasit:master Apr 14, 2023
1 check passed
@wanasit
Copy link
Owner

wanasit commented Jul 13, 2023

I apologize that I forgot to publish the new version for months.
This change has been included in v2.6.4.

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.

None yet

2 participants