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

ignore min length for term with char in config #19

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

BlueGreenMagick
Copy link

@BlueGreenMagick BlueGreenMagick commented Aug 11, 2019

Description

Concisely describe what the pull request is trying to achieve. If pertinent, link to an existing issue report, or briefly explain the problem the PR is meant to solve. Feel free to attach screenshots or other media for UI-related changes.

Allows users to specify a regex character set in config. When checking if length of the term is less than 3 and not providing dictionary if so, checks if the term contains any character in the character set and provide dictionary if so.

Useful for languages such as Korean, or Chinese, where there are many 2 length and 1 length words. (While English has only 26 possible 1 length letters, There are 11,172 possible 1 length letters in Korean for example)

Here is an example value of the config ignoreMinLength:
[\u1100-\u11FF\u3130-\u318F\uA960-\uA97F\uAC00-\uD7AF\uD7B0-\uD7FF]

Checklist:

Please replace the space inside the brackets with an x and fill out the ellipses if the following items apply:

  • I've read and understood the contribution guidelines
  • I've tested my changes against at least one of the following Anki builds:
    • Latest standard Anki 2.1 binary build [required for Anki-compatible 2.1 add-ons]
    • Latest alternative Anki 2.1 binary build
    • Latest Anki 2.0 binary build [required for Anki 2.0-compatible add-ons]
  • I've tested my changes on at least one of the following platforms:
    • Linux, version:
    • Windows, version: 10
    • macOS, version:
  • My changes potentially affect non-desktop platforms, of which I've tested:
    • AnkiMobile, version:
    • AnkiDroid, version:
    • AnkiWeb

@glutanimate
Copy link
Owner

Thanks, this seems like a great way to address #16. I like the general idea of the approach and implementation, however right now it seems like leaving ignoreMinLength empty causes the add-on to malfunction with:

JS error on line 253: Uncaught SyntaxError: Unexpected token if

image

I assume it's because the regex literal remains empty.

If you can find a way to fix that I'd be happy to merge this change.

Thanks again for taking the time to contribute, and documenting your reasoning and changes in such an exemplary way!

src/popup_dictionary/web.py Outdated Show resolved Hide resolved
@BlueGreenMagick
Copy link
Author

Thanks for the feedback. I'm not sure why I haven't thought of testing it with empty string. I modified the code, would this be alright?

@glutanimate
Copy link
Owner

Hey, sorry for taking a while to get back to you. Have been busy with a lot of other things recently.

Thanks for updating the PR regarding the aforementioned issue. The problem is that I've refactored the add-on structure in the meanwhile, and so I can no longer easily merge our changes. To be honest, this is on me as I forgot to look at the existing PRs before refactoring the add-on. Either way, I was wondering what your preferred way forward was?

I could try to manually work in your changes on my side of things and credit you in the commit messages. But if you have the time to adjust your changes to the new add-on structure, I'd be happy to merge them as part of a new PR, too.

Just let me know what you prefer and we'll do it that way!

@BlueGreenMagick
Copy link
Author

Is the refactored add-on in working state? I edited my changes, but I could not test it as I was unable to get a popup window open even without my changes, no other add-ons enabled.

@glutanimate
Copy link
Owner

Ah, yes, sorry. I forgot to push a couple of commits. Please try again with the latest master.

@BlueGreenMagick
Copy link
Author

The latest master worked. I tested my changes and found no issues. Let me know if there are any problems or if anything can be improved.

@glutanimate glutanimate self-assigned this Aug 1, 2021
@glutanimate glutanimate added this to the v1.1.0 milestone Aug 1, 2021
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