Add a functional test for the spelling checker

[Pierre-Sassoulas]

Type of Changes

	Type
✓	✨ New feature

Description

Add a spelling functional tests:

Currently this fail with:

Traceback (most recent call last):
  File "/home/pierre/pylint/venv/bin/pylint", line 11, in <module>
    load_entry_point('pylint', 'console_scripts', 'pylint')()
  File "/home/pierre/pylint/pylint/__init__.py", line 22, in run_pylint
    PylintRun(argv or sys.argv[1:])
  File "/home/pierre/pylint/pylint/lint/run.py", line 350, in __init__
    args = _config_initialization(
  File "/home/pierre/pylint/pylint/config/config_initialization.py", line 59, in _config_initialization
    linter.load_plugin_modules(plugins)
  File "/home/pierre/pylint/pylint/lint/pylinter.py", line 644, in load_plugin_modules
    module.register(self)
  File "/home/pierre/pylint/pylint/checkers/spelling.py", line 449, in register
    linter.register_checker(SpellingChecker(linter))
  File "/home/pierre/pylint/pylint/lint/pylinter.py", line 768, in register_checker
    self.register_options_provider(checker)
  File "/home/pierre/pylint/pylint/config/option_manager_mixin.py", line 94, in register_options_provider
    self.add_option_group(
  File "/home/pierre/pylint/pylint/config/option_manager_mixin.py", line 131, in add_option_group
    self.add_optik_option(provider, group, opt, optdict)
  File "/home/pierre/pylint/pylint/config/option_manager_mixin.py", line 135, in add_optik_option
    option = optikcontainer.add_option(*args, **optdict)
  File "/usr/lib/python3.8/optparse.py", line 1008, in add_option
    self._check_conflict(option)
  File "/usr/lib/python3.8/optparse.py", line 980, in _check_conflict
    raise OptionConflictError(
optparse.OptionConflictError: option --spelling-dict: conflicting option string(s): --spelling-dict

In order to be able to add a mypy example in #5929

[Pierre-Sassoulas]

@DanielNoord could you have a look, please ? I think it shows a regression from the argparse migration.

[DanielNoord]

@DanielNoord could you have a look, please ? I think it shows a regression from the argparse migration.

Are you sure? This is an optparse error.

But I'll have a look!

[Pierre-Sassoulas]

It's entirely possible that it's a pre-existing issue 😄

[DanielNoord]

@Pierre-Sassoulas Uhm, what is the bug here? spelling.py is not an extension but a standard module. So trying to load it as a plugin is rightfully warning about double options: it tries to load the option of the "plugin" spelling.py over the "normal" spelling.py?

[Pierre-Sassoulas]

Hmm, thank you I jumped to conclusion sorry. Well I don't know how to enable this checker then. I'll investigate.

[jacobtylerwalls]

Have you tried specifying a dictionary?

diff --git a/pylintrc b/pylintrc
index c4816236..b9aa2292 100644
--- a/pylintrc
+++ b/pylintrc
@@ -423,7 +423,7 @@ missing-member-max-choices=1
 
 # Spelling dictionary name. Available dictionaries: none. To make it working
 # install python-enchant package.
-spelling-dict=
+spelling-dict=en-US
 
 # List of comma separated words that should not be checked.
 spelling-ignore-words=

[Pierre-Sassoulas]

Thank you @jacobtylerwalls it worked !

[coveralls]

Pull Request Test Coverage Report for Build 2897447512

• 8 of 11 (72.73%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.0003%) to 95.255%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pylint/checkers/spelling.py	8	11	72.73%

Totals
Change from base Build 2897313644:	0.0003%
Covered Lines:	16882
Relevant Lines:	17723

---

💛 - Coveralls

[DanielNoord]

Do we understand why we get None:None here on 3.10?

[Pierre-Sassoulas]

I think it's the same on 3.7, 3.8, 3.9.. Probably because we're passing the line as an int instead of a node.

[DanielNoord]

Do we want to fix this in a follow-up PR? Or in this PR?

[Pierre-Sassoulas]

I want to do two:

• Pass node instead of line number as int
• Add pylint's msgids and symbols to the dictionary automatically

Probably better after we merge this one and #5929 though, we're very bsy right now 😄

[DanielNoord]

Agreed! Could you make an issue about passing node instead of line in this checker though? So we don't forget about it!

[Pierre-Sassoulas]

#6416

[Pierre-Sassoulas]

We're impacted by #5092 on pypy and windows, but pyenchant should have been installed.

[DanielNoord]

@Pierre-Sassoulas What is the state of this PR? Note that there is a unittest_spelling test in the CI? Can't we put this there?

[Pierre-Sassoulas]

Note that there is a unittest_spelling test in the CI? Can't we put this there?

The idea is to have an example of functional tests for the spelling checker so it's easier to add functional tests for contributors. I'm going to do a refactor to initialize variable in the constructor of the spelling checker and then rebase this one on it.

[DanielNoord]

Note that there is a unittest_spelling test in the CI? Can't we put this there?

The idea is to have an example of functional tests for the spelling checker so it's easier to add functional tests for contributors. I'm going to do a refactor to initialize variable in the constructor of the spelling checker and then rebase this one on it.

Just thought about this some more: let's not do this.

We can't really control which dictionary people will install locally. That's (imo) a bad design in enchant but en can point to any form of English dictionary you have installed locally. I could even create a Dutch dictionary and refer to it as en. This creates a high risk of locally broken CI's due to different versions of dictionaries being available (there is a difference on macOS and Ubuntu for example). It also adds another hurdle to contributing to pylint (installing a dictionary and the enchant package).
Let's keep running the spelling tests, but only in the current CI step where we can control the dictionary to be used.

[Pierre-Sassoulas]

Could we use our own fake dictionary for functional tests ? Beside there's the same issue in the unittest, right ?

[DanielNoord]

Could we use our own fake dictionary for functional tests ? Beside there's the same issue in the unittest, right ?

Yeah we currently skip those when we don't find the en_US dictionary. So, there is still the risk of differing local dictionaries.

The only thing we could try is whether it works to set dictionary="" and then add a user-defined dictionary for the functional tests. We could then only use words in that dictionary, but that might work. Still, I think I don't think trying this should be blocking 2.14 and #5929, which could just be added to the unittests.

[Pierre-Sassoulas]

Let's move this to 2.15.

[DanielNoord]

@Pierre-Sassoulas It's a bit much for one test... But this works 😅

[DanielNoord]

Okay, I'm retracting that.

Let's not do this. "" doesn't work as we check if there is an actual dictionary in the option and not just an empty string.
But even if we handle the dictionary not being installed we don't control the content of the dictionary, as the differences between Linux and Windows show.

Let's just use the exisiting unittests for this: we control the dictionary there and it is also much easier to skip if the dictionary is missing compared to the 20+ lines needed for this to work in the functional tests.

[Pierre-Sassoulas]

I have a plan to make this work, but I'm working on low hanging fruits right now 😄

[DanielNoord]

I have a plan to make this work, but I'm working on low hanging fruits right now 😄

I'm interested to see what you can come up with 😄

[Pierre-Sassoulas]

I'm interested to see what you can come up with

I should have talked about it because now I completely forgot what the plan was

[Pierre-Sassoulas]

When rebasing I realized that the dict is changing depending on the operating system (even ubuntu 18.4 vs 20.4), so the suggestion changes. I guess the only solution would be to add our own dict, but let's use the unit test where it's easy to define our own dict instead (as you said).

[github-actions]

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit cafdc37