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

[spelling checker] Better help message and code organization #8338

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 35 additions & 17 deletions pylint/checkers/spelling.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -35,8 +35,11 @@
WikiWordFilter,
get_tokenizer,
)
except ImportError:

PYENCHANT_AVAILABLE = True
except ImportError: # pragma: no cover
enchant = None
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
enchant = None

Does this line serve a purpose anymore now that the PYENCHANT_AVAILABLE is here?

Copy link
Member Author

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.

PYENCHANT_AVAILABLE = False

class EmailFilter: # type: ignore[no-redef]
...
Expand All @@ -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]]:
Copy link
Member

Choose a reason for hiding this comment

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

I was a bit confused by the use of the word dicts while a list of tuples are returned - but this is enchant terminology?

Copy link
Member Author

Choose a reason for hiding this comment

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

enchant.ProviderDesc are the dict, as in an actual dict with word in it, for the spelling check, not a python data structure.

# 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]
Copy link
Member

Choose a reason for hiding this comment

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

Is the [""] a default - would this work also? Feel free to leave as-is 🕺

Suggested change
return [""] + [d[0] for d in inner_enchant_dicts]
return [d[0] for d in inner_enchant_dicts] or [""]

Copy link
Member Author

@Pierre-Sassoulas Pierre-Sassoulas Feb 27, 2023

Choose a reason for hiding this comment

The 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]
Expand Down Expand Up @@ -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),
},
),
(
Expand Down Expand Up @@ -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:
Expand Down
15 changes: 15 additions & 0 deletions tests/checkers/unittest_spelling.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import pytest

from pylint.checkers import spelling
from pylint.checkers.spelling import _get_enchant_dict_help
from pylint.testutils import CheckerTestCase, MessageTest, _tokenize_str, set_config

# try to create enchant dictionary
Expand Down Expand Up @@ -39,6 +40,20 @@ def _get_msg_suggestions(self, word: str, count: int = 4) -> str:
suggestions = "' or '".join(self.checker.spelling_dict.suggest(word)[:count])
return f"'{suggestions}'"

def test_spelling_dict_help_no_enchant(self) -> None:
assert "both the python package and the system dep" in _get_enchant_dict_help(
[], pyenchant_available=False
)
assert "need to install the system dep" in _get_enchant_dict_help(
[], pyenchant_available=True
)

@skip_on_missing_package_or_dict
def test_spelling_dict_help_enchant(self) -> None:
assert "Available dictionaries: " in _get_enchant_dict_help(
enchant.Broker().list_dicts(), pyenchant_available=True
)

@skip_on_missing_package_or_dict
@set_config(spelling_dict=spell_dict)
def test_check_bad_coment(self) -> None:
Expand Down