allow non-ASCII characters in identifiers in the invalid-name rule and add non-ascii-name #3409

bfgray3 · 2020-02-14T01:28:40Z

Steps

Add yourself to CONTRIBUTORS if you are a new contributor.
Add a ChangeLog entry describing what your PR does.
If it's a new feature or an important bug fix, add a What's New entry in doc/whatsnew/<current release.rst>.
Write a good description on what the PR does.

Description

This PR attempts to allow non-ASCII characters in identifiers in the invalid-name rule.

Type of Changes

	Type
✓	✨ New feature

Related Issue

This PR is in reference to issue #2725 but I have not closed it in the commit message yet because I have not yet implemented the second part and there are likely more changes required to this PR as well.

PCManticore · 2020-02-14T08:31:12Z

Hey @bfgray3 Thank you for working on this, looks great from the start! Let me know if you want to continue this PR for adding the new check or if you want to add it in a separate PR. Both alternatives work for me!

bfgray3 · 2020-02-16T03:34:59Z

I'll continue on this PR--I'll push some more commits in the next few days 👍

bfgray3 · 2020-02-17T02:42:02Z

@PCManticore I pushed some more commits for a first stab at non-ascii-name but it's pretty rough right now. I'm sure the message ID (C0144 in these commits) will have to change and more tests are needed (and probably some refactoring too) but I wanted to get some feedback. Thanks!

coveralls · 2020-02-17T02:46:13Z

Coverage decreased (-0.04%) to 90.354% when pulling ad4aebb on bfgray3:non-ascii-invalid-name into fb38afb on PyCQA:master.

coveralls · 2020-02-17T02:46:14Z

Coverage increased (+0.003%) to 90.393% when pulling 2cd7b8c on bfgray3:non-ascii-invalid-name into 0504213 on PyCQA:master.

PCManticore

Hey @bfgray3 Overall this looks great, thank you! Left just a comment to be addressed before merging this. Is this ready to be merged or do you plan to add some other tests? Also the error code looks fine to me.

PCManticore · 2020-02-27T08:54:00Z

pylint/checkers/base.py

@@ -1955,6 +1972,14 @@ def _name_invalid_due_to_blacklist(self, name: str) -> bool:
    def _check_name(self, node_type, name, node, confidence=interfaces.HIGH):
        """check for a name using the type's regexp"""

+        non_ascii_regexp = re.compile("[^\u0000-\u007F]")


Can you move the compilation at __init__ level? This way we don't have to compile a regex for every name we analyze.

bfgray3 · 2020-02-28T02:57:09Z

Hey @bfgray3 Overall this looks great, thank you! Left just a comment to be addressed before merging this. Is this ready to be merged or do you plan to add some other tests? Also the error code looks fine to me.

I think this should be ready to merge, the only thing I was wondering was whether or not the regexps here need to be updated.

PCManticore · 2020-02-28T08:57:30Z

@bfgray3 Good catch, we should also fix those. Also it seems that the tests are failing on AppVeyor, can you take a look on why that happens?

bfgray3 · 2020-03-01T01:46:17Z

@PCManticore I took a look at the AppVeyor logs and saw the following. I wonder if Windows doesn't always handle unicode correctly. Not sure.

================================== FAILURES ===================================
_______________________ test_functional[non_ascii_name] _______________________
test_file = FunctionalTest:non_ascii_name
    @pytest.mark.parametrize("test_file", TESTS, ids=TESTS_NAMES)
    def test_functional(test_file):
        LintTest = (
            LintModuleOutputUpdate(test_file) if UPDATE else testutils.LintModuleTest(test_file)
        )
        LintTest.setUp()
>       LintTest._runTest()
..\tests\test_functional.py:88: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
py37\lib\site-packages\pylint\testutils.py:599: in _runTest
    self._check_output_text(expected_messages, expected_text, received_text)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
self = <pylint.testutils.LintModuleTest object at 0x0000004DEDCA6888>
expected_messages = Counter({(3, 'non-ascii-name'): 1, (5, 'non-ascii-name'): 1})
expected_lines = [OutputLine(symbol='non-ascii-name', lineno=3, object='', msg='Constant name "áé�\xadóú" contains a non-ASCII unic...object='úó�\xadéá', msg='Function name "úó�\xadéá" contains a non-ASCII unicode character', confidence='HIGH')]
received_lines = [OutputLine(symbol='non-ascii-name', lineno=3, object='', msg='Constant name "�����" contains a non-ASCII unicode char...ame', lineno=5, object='�����', msg='Function name "�����" contains a non-ASCII unicode character', confidence='HIGH')]
    def _check_output_text(self, expected_messages, expected_lines, received_lines):
        assert (
            self._split_lines(expected_messages, expected_lines)[0] == received_lines
>       ), "Error with the following functional test: {}".format(self._test_file.base)
E       AssertionError: Error with the following functional test: non_ascii_name
py37\lib\site-packages\pylint\testutils.py:614: AssertionError
=== 1 failed, 1133 passed, 47 skipped, 201 deselected in 114.20s (0:01:54) ====
ERROR: InvocationError for command 'C:\projects\pylint\.tox\py37\Scripts\python.EXE' -Wi 'py37\Lib\site-packages\coverage' run -m pytest 'C:\projects\pylint/tests/' (exited with code 1)
___________________________________ summary ___________________________________
ERROR:   py37: commands failed
Command exited with code 1

PCManticore · 2020-03-10T10:46:55Z

@bfgray3 It's entirely possible that's the case. Other users seem to complain about this as well in https://help.appveyor.com/discussions/problems/6245-unicode-characters-show-as-in-build-output.

Let's try something, can you add the following - ps: chcp 65001 in https://github.com/PyCQA/pylint/blob/master/appveyor.yml#L16? If that makes it work and no other tests are failing, we can keep that command. Otherwise, we'll have to skip these tests on Windows.

Let me know how that sounds.

bfgray3 · 2020-03-11T01:35:14Z

@bfgray3 It's entirely possible that's the case. Other users seem to complain about this as well in https://help.appveyor.com/discussions/problems/6245-unicode-characters-show-as-in-build-output.

Let's try something, can you add the following - ps: chcp 65001 in https://github.com/PyCQA/pylint/blob/master/appveyor.yml#L16? If that makes it work and no other tests are failing, we can keep that command. Otherwise, we'll have to skip these tests on Windows.

Let me know how that sounds.

@PCManticore I tried the ps command in ad4aebb but it didn't work, so I ended up skipping the tests on Windows and that seemed to do the trick. The way I did the skipping is pretty inelegant; please let me know if there's a better way.

PCManticore · 2020-03-11T13:13:55Z

Thank you @bfgray3 for the PR! 🎉

bfgray3 added 3 commits February 13, 2020 20:14

first pass at allowing non-ascii characters in invalid-name; see #2725

d159cc0

add non-ascii invalid name note to whatsnew

ba304fb

use r strings for regex values

ff35831

bfgray3 added 6 commits February 16, 2020 19:29

start work on non-ascii-name

8de837e

add empty test files for non-ascii-name

4cb61dd

first pass at non-ascii-name

3f83cb5

refactor non-ascii-name

ac8ca2c

add note to ChangeLog about non-ascii-name

03904a3

add non-ascii-name note to whatsnew

2cd7b8c

bfgray3 changed the title ~~allow non-ASCII characters in identifiers in the invalid-name rule~~ allow non-ASCII characters in identifiers in the invalid-name rule and add non-ascii-name Feb 17, 2020

PCManticore approved these changes Feb 27, 2020

View reviewed changes

move regex compilation to __init__()

39a5107

bfgray3 added 3 commits February 29, 2020 19:49

merge upstream master

f5ec31c

update regexps in pyreverse/utils.py

5da7154

use raw strings for regexps

2eee614

bfgray3 added 3 commits March 10, 2020 20:07

add '- ps: chcp 65001' to appveyor.yml

ad4aebb

skip non_ascii_name.py tests on windows

b6d0440

remove '- ps: chcp 65001' from appveyor.yml

f76c342

PCManticore merged commit 762e1a1 into pylint-dev:master Mar 11, 2020

PCManticore mentioned this pull request Mar 11, 2020

invalid-name does not recognize identifiers with non-ASCII characters #2725

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow non-ASCII characters in identifiers in the invalid-name rule and add non-ascii-name #3409

allow non-ASCII characters in identifiers in the invalid-name rule and add non-ascii-name #3409

bfgray3 commented Feb 14, 2020

PCManticore commented Feb 14, 2020

bfgray3 commented Feb 16, 2020

bfgray3 commented Feb 17, 2020

coveralls commented Feb 17, 2020 •

edited

Loading

coveralls commented Feb 17, 2020

PCManticore left a comment

PCManticore Feb 27, 2020

bfgray3 Feb 28, 2020

bfgray3 commented Feb 28, 2020

PCManticore commented Feb 28, 2020

bfgray3 commented Mar 1, 2020

PCManticore commented Mar 10, 2020

bfgray3 commented Mar 11, 2020 •

edited

Loading

PCManticore commented Mar 11, 2020

allow non-ASCII characters in identifiers in the invalid-name rule and add non-ascii-name #3409

allow non-ASCII characters in identifiers in the invalid-name rule and add non-ascii-name #3409

Conversation

bfgray3 commented Feb 14, 2020

Steps

Description

Type of Changes

Related Issue

PCManticore commented Feb 14, 2020

bfgray3 commented Feb 16, 2020

bfgray3 commented Feb 17, 2020

coveralls commented Feb 17, 2020 • edited Loading

coveralls commented Feb 17, 2020

PCManticore left a comment

Choose a reason for hiding this comment

PCManticore Feb 27, 2020

Choose a reason for hiding this comment

bfgray3 Feb 28, 2020

Choose a reason for hiding this comment

bfgray3 commented Feb 28, 2020

PCManticore commented Feb 28, 2020

bfgray3 commented Mar 1, 2020

PCManticore commented Mar 10, 2020

bfgray3 commented Mar 11, 2020 • edited Loading

PCManticore commented Mar 11, 2020

coveralls commented Feb 17, 2020 •

edited

Loading

bfgray3 commented Mar 11, 2020 •

edited

Loading