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

Add functions to convert kansuji in the string to Arabic numbers. #23

Merged
merged 9 commits into from Nov 11, 2020
Merged

Add functions to convert kansuji in the string to Arabic numbers. #23

merged 9 commits into from Nov 11, 2020

Conversation

indenkun
Copy link
Contributor

@indenkun indenkun commented Nov 9, 2020

漢数字(複数桁)の変換を含めて、漢数字からアラビア数字への変換について解決できそうな案(関数)について提案します。

kansuji2arabic_numで京までの十や百などの位を表す漢数字を含む漢数字を既存のkansuji2arabic_allでは置換する形になるもの(例:二千二十が21000210になる)を、漢数字で表している数となるように計算するようになります(例:二千二十が2020になる)。

kansuji2arabic_strで文中の漢数字を上記のように計算した上でアラビア数字に変換する関数です。

ContributingやPull request templateがなくどう提案するかわからなかったので、不適切な提案でしたら破棄してください。

#8

@uribo
Copy link
Owner

uribo commented Nov 10, 2020

素晴らしい実装をありがとうございます。
NEWSファイルに変更内容を簡潔に記載いただけますと幸いです。

また、差し支えなければパッケージのcontributorに加えさせていただきたく、DESCRIPTIONへの追加コミットをお願いできますでしょうか。
例) 79feb30

@indenkun
Copy link
Contributor Author

ありがとうございます。
コード内に若干の不具合があったので修正とコード内でstats::na.omitを使っているのでDescriptionのimportに{stats}を追加しました。

テストも追加したほうが良いでしょうか。

@uribo
Copy link
Owner

uribo commented Nov 10, 2020

対応いただきまして、ありがとうございます。
そうですね。@examplesに書いているコードを含めた簡単なテストを用意いただけますと助かります。

)
expect_equal(
kansuji2arabic_str("\u91d1\u4e00\u5104\u4e8c\u5343\u4e09\u767e\u56db\u5341\u4e94\u4e07\u516d\u5343\u4e03\u767e\u516b\u5341\u4e5d\u5186"),
"\\u91d1123456789\\u5186"
Copy link
Owner

Choose a reason for hiding this comment

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

ここのunicodeエスケープは \一つで十分です。 "\u91d1123456789\u5186"

)
expect_equal(
kansuji2arabic_str("\u91d1\u4e00\u5104\u4e8c\u4e09\u56db\u4e94\u4e07\u516d\u4e03\u516b\u4e5d\u5186"),
"\\u91d1123456789\\u5186"
Copy link
Owner

Choose a reason for hiding this comment

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

)
expect_equal(
kansuji2arabic_str("\u91d11\u51042345\u4e076789\u5186"),
"\\u91d1123456789\\u5186"
Copy link
Owner

Choose a reason for hiding this comment

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

@indenkun
Copy link
Contributor Author

ありがとうございます。

自分でも、R-CMD-checkdeでエラーがでているのに気づいて直後に修正したコードをフォークしているリポジトリにはMergeしたのですが、うまくこのPull Requestに載せられておらず(自分のgithubの操作の理解が不十分なせいです)、すみません。確認中です。

@uribo
Copy link
Owner

uribo commented Nov 11, 2020

6ab6f3a こちらのコミットで対応されていましたね!ありがとうございます。

@uribo uribo merged commit 181f9f8 into uribo:master Nov 11, 2020
@indenkun
Copy link
Contributor Author

Pull Requestできない理由がわからなかったので、ごくごく軽微なその他のファイルの修正(空行の削除)をマージしたら一緒に前のファイルもPull Request通りました。
お手数かけました。

Mergeしていただきありがとうございます。

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