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

--lang option #55

Merged
merged 18 commits into from
Apr 4, 2022
Merged

--lang option #55

merged 18 commits into from
Apr 4, 2022

Conversation

eggplants
Copy link
Contributor

I have added -l, --lang option to select a model by a language code.


$ budoux -l aa
usage: budoux [-h] [-H] [-m JSON | -l LANG] [-d STR] [-t THRES] [-V] [TXT]
budoux: error: argument -l/--lang: 'aa' does not exist in builtin models. (supported languages: ['zh', 'ja'])

$ budoux -l zh 今天天气晴朗。
今天天气
晴朗。

$ budoux -l ja -m budoux/models/ja-knbc.json 今日はいい天気ですね。
usage: budoux [-h] [-H] [-m JSON | -l LANG] [-d STR] [-t THRES] [-V] [TXT]
budoux: error: argument -m/--model: not allowed with argument -l/--lang

Copy link
Member

@tushuhei tushuhei left a comment

Choose a reason for hiding this comment

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

Thank you for your proposal! I think this is a great idea to add the lang option to the CLI. Some change requests below:

.gitignore Outdated
@@ -6,6 +6,9 @@ __pycache__
*.pyc
*.log
*.egg-info
*,cover
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this comma should be a dot instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I executed pytest --cov-report annotate, somehow *,cover files were generated. But it is not generated when I run it now. So I'll delete this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

README.md Outdated
本日は
晴天です。

$ budoux -l zh 今天天气晴朗。
Copy link
Member

Choose a reason for hiding this comment

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

Actually, I want to use zh-hans as the language code because I would like to emphasize that this is for Simplified Chinese (hans), not for Traditional Chinese (hant) here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

README.md Outdated
-h, --help show this help message and exit
-H, --html HTML mode (default: False)
-m JSON, --model JSON custom model file path (default: /path/to/models/ja-knbc.json)
-m JSON, --model JSON custom model file path (default: /home/eggplants/prog/budoux/budoux/models/ja-knbc.json)
Copy link
Member

Choose a reason for hiding this comment

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

Probably we may want to generalize the default model path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

README.md Outdated
-d STR, --delim STR output delimiter in TEXT mode (default: ---)
-t THRES, --thres THRES threshold value to separate chunks (default: 1000)
-V, --version show program's version number and exit

supported languages of `-l`, `--lang`:
- zh
Copy link
Member

Choose a reason for hiding this comment

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

I would like to use zh-hans here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

budoux/main.py Show resolved Hide resolved
@eggplants eggplants requested a review from tushuhei April 4, 2022 01:06
Copy link
Member

@tushuhei tushuhei left a comment

Choose a reason for hiding this comment

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

Thank you for the fix!

@tushuhei tushuhei merged commit 283bb5b into google:main Apr 4, 2022
@eggplants eggplants deleted the lang_option branch April 4, 2022 03:56
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.

2 participants