-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[spelling checker] Better help message and code organization #8338
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -10,7 +10,7 @@ | |||||
import sys | ||||||
import tokenize | ||||||
from re import Pattern | ||||||
from typing import TYPE_CHECKING | ||||||
from typing import TYPE_CHECKING, Any | ||||||
|
||||||
from astroid import nodes | ||||||
|
||||||
|
@@ -35,8 +35,11 @@ | |||||
WikiWordFilter, | ||||||
get_tokenizer, | ||||||
) | ||||||
except ImportError: | ||||||
|
||||||
PYENCHANT_AVAILABLE = True | ||||||
except ImportError: # pragma: no cover | ||||||
enchant = None | ||||||
PYENCHANT_AVAILABLE = False | ||||||
|
||||||
class EmailFilter: # type: ignore[no-redef] | ||||||
... | ||||||
|
@@ -62,17 +65,33 @@ def get_tokenizer( | |||||
return Filter() | ||||||
|
||||||
|
||||||
if enchant is not None: | ||||||
br = enchant.Broker() | ||||||
dicts = br.list_dicts() | ||||||
dict_choices = [""] + [d[0] for d in dicts] | ||||||
dicts = [f"{d[0]} ({d[1].name})" for d in dicts] | ||||||
dicts = ", ".join(dicts) | ||||||
instr = "" | ||||||
else: | ||||||
dicts = "none" | ||||||
dict_choices = [""] | ||||||
instr = " To make it work, install the 'python-enchant' package." | ||||||
def _get_enchant_dicts() -> list[tuple[Any, enchant.ProviderDesc]]: | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was a bit confused by the use of the word There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||
# Broker().list_dicts() is not typed in enchant, but it does return tuples | ||||||
return enchant.Broker().list_dicts() if PYENCHANT_AVAILABLE else [] # type: ignore[no-any-return] | ||||||
|
||||||
|
||||||
def _get_enchant_dict_choices( | ||||||
inner_enchant_dicts: list[tuple[Any, enchant.ProviderDesc]] | ||||||
) -> list[str]: | ||||||
return [""] + [d[0] for d in inner_enchant_dicts] | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, basically an empty dict name means you're not checking anything it's the default value. But when some dict are installed you get a list like ["", "en", "gb"], and you need the empty value. |
||||||
|
||||||
|
||||||
def _get_enchant_dict_help( | ||||||
inner_enchant_dicts: list[tuple[Any, enchant.ProviderDesc]], | ||||||
pyenchant_available: bool, | ||||||
) -> str: | ||||||
if inner_enchant_dicts: | ||||||
dict_as_str = [f"{d[0]} ({d[1].name})" for d in inner_enchant_dicts] | ||||||
enchant_help = f"Available dictionaries: {', '.join(dict_as_str)}" | ||||||
else: | ||||||
enchant_help = "No available dictionaries : You need to install " | ||||||
if not pyenchant_available: | ||||||
enchant_help += "both the python package and " | ||||||
enchant_help += "the system dependency for enchant to work." | ||||||
return f"Spelling dictionary name. {enchant_help}." | ||||||
|
||||||
|
||||||
enchant_dicts = _get_enchant_dicts() | ||||||
|
||||||
|
||||||
class WordsWithDigitsFilter(Filter): # type: ignore[misc] | ||||||
|
@@ -237,9 +256,8 @@ class SpellingChecker(BaseTokenChecker): | |||||
"default": "", | ||||||
"type": "choice", | ||||||
"metavar": "<dict name>", | ||||||
"choices": dict_choices, | ||||||
"help": "Spelling dictionary name. " | ||||||
f"Available dictionaries: {dicts}.{instr}", | ||||||
"choices": _get_enchant_dict_choices(enchant_dicts), | ||||||
"help": _get_enchant_dict_help(enchant_dicts, PYENCHANT_AVAILABLE), | ||||||
}, | ||||||
), | ||||||
( | ||||||
|
@@ -297,7 +315,7 @@ class SpellingChecker(BaseTokenChecker): | |||||
|
||||||
def open(self) -> None: | ||||||
self.initialized = False | ||||||
if enchant is None: | ||||||
if not PYENCHANT_AVAILABLE: | ||||||
return | ||||||
dict_name = self.linter.config.spelling_dict | ||||||
if not dict_name: | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this line serve a purpose anymore now that the
PYENCHANT_AVAILABLE
is here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it's basically so it's defined and we don't get screamed at by linters 😅 The design of this is peculiar, I'm thinking an external plugin would make more sense, because then it could convey in pip metadata that it NEED py-enchant and we could remove all of this. Maybe something to do as a breaking change in 4.0.