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 option to disable tone coloring to EDICT2 plug-in #590

Merged
merged 9 commits into from
Oct 23, 2024
Merged

Add option to disable tone coloring to EDICT2 plug-in #590

merged 9 commits into from
Oct 23, 2024

Conversation

a1ess
Copy link
Contributor

@a1ess a1ess commented Oct 22, 2024

This is the pull request for issue 589.

I have added the new boolean option colorize_tones, to turn off tone coloring when generating the converted dictionary. As before the plug-in generated colorized tones, I set the option to True by default.

I have run ruff check as requested, and got two warnings for the file conv.py: "too-many-arguments (PLR0913)", as with the new option some of the functions have six arguments instead of the five maximum arguments set by default in ruff.

a1ess added 2 commits October 22, 2024 23:10
Added colorize_tones option
if len(syllables) != len(tones):
log.warning(f"unmatched tones: {syllables=}, {tones=}")

if (len(syllables) != len(tones)) or (colorize_tones is False):
Copy link
Owner

Choose a reason for hiding this comment

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

if len(syllables) != len(tones) or not colorize_tones:


if (len(syllables) != len(tones)) or (colorize_tones is False):

if (len(syllables) != len(tones)):
Copy link
Owner

@ilius ilius Oct 22, 2024

Choose a reason for hiding this comment

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

Extra parenthesis.

Copy link
Owner

Choose a reason for hiding this comment

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

You check len(syllables) != len(tones) twice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The second check is to log the "unmatched tones" warning, as the first condition catches this case and the option to disable tone coloring. I can eliminate the condition, but the warning will be logged also when only colorized_tones is False. I couldn't come up with a better solution.

@ilius
Copy link
Owner

ilius commented Oct 22, 2024

You didn't add the changes in doc and plugin-meta.

And run: ruff check.

Ignore the Too many arguments error by adding # noqa: PLR0913 comment

a1ess added 3 commits October 23, 2024 01:07
Updated with new option of EDIC2 plugin
Updated with new option of EDICT2 plugin
Added comments to avoid warning PLR0913 from ruff
@ilius
Copy link
Owner

ilius commented Oct 22, 2024

Are you using Windows?

@ilius
Copy link
Owner

ilius commented Oct 22, 2024

Please try git pull, then ./scripts/gen again, add changes and commit and push.

@a1ess
Copy link
Contributor Author

a1ess commented Oct 23, 2024

Thank you for your help, it should be fine now. I'm on Linux, just don't usually use GitHub.

@ilius ilius merged commit a934ff7 into ilius:master Oct 23, 2024
9 checks passed
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