Add UseAssertIn lint rule to Fixit #159

dkteo · 2020-11-30T22:23:56Z

Summary

Adding the Python lint rule for assertIn/assertNotIn.
The reason we want to do this is to warn users to not make use of assertTrue(x in y)/assertFalse(x in y) which is deprecated in Py3.8. See deprecated rules

Test Plan

Ran tox -e py38 -- fixit.tests.UseAssertInRule

GLOB sdist-make: /Users/dkteo/Fixit/setup.py
py37 inst-nodeps: /Users/dkteo/Fixit/.tox/.tmp/package/1/fixit-0.1.2.zip
py37 installed: alabaster==0.7.12,appdirs==1.4.4,appnope==0.1.0,argon2-cffi==20.1.0,async-generator==1.10,attrs==20.3.0,Babel==2.9.0,backcall==0.2.0,black==20.8b1,bleach==3.2.1,certifi==2020.11.8,cffi==1.14.4,chardet==3.0.4,click==7.1.2,codecov==2.1.10,coverage==5.3,decorator==4.4.2,defusedxml==0.6.0,diff-cover==4.0.1,distlib==0.3.1,docutils==0.16,entrypoints==0.3,filelock==3.0.12,fixit @ file:///Users/dkteo/Fixit/.tox/.tmp/package/1/fixit-0.1.2.zip,flake8==3.8.4,idna==2.10,imagesize==1.2.0,importlib-metadata==3.1.0,inflect==5.0.2,ipykernel==5.3.4,ipython==7.19.0,ipython-genutils==0.2.0,ipywidgets==7.5.1,isort==5.6.4,jedi==0.17.2,Jinja2==2.11.2,jinja2-pluralize==0.3.0,jsonschema==3.2.0,jupyter==1.0.0,jupyter-client==6.1.7,jupyter-console==6.2.0,jupyter-core==4.7.0,jupyterlab-pygments==0.1.2,libcst==0.3.14,MarkupSafe==1.1.1,mccabe==0.6.1,mistune==0.8.4,mypy-extensions==0.4.3,nbclient==0.5.1,nbconvert==6.0.7,nbformat==5.0.8,nbsphinx==0.8.0,nest-asyncio==1.4.3,notebook==6.1.5,packaging==20.7,pandocfilters==1.4.3,parso==0.7.1,pathspec==0.8.1,pexpect==4.8.0,pickleshare==0.7.5,pluggy==0.13.1,prometheus-client==0.9.0,prompt-toolkit==3.0.8,psutil==5.7.3,ptyprocess==0.6.0,py==1.9.0,pycodestyle==2.6.0,pycparser==2.20,pyflakes==2.2.0,Pygments==2.7.2,pyparsing==2.4.7,pyre-check==0.0.41,pyre-extensions==0.0.19,pyrsistent==0.17.3,python-dateutil==2.8.1,pytz==2020.4,pywatchman==1.4.1,PyYAML==5.3.1,pyzmq==20.0.0,qtconsole==5.0.1,QtPy==1.9.0,regex==2020.11.13,requests==2.25.0,Send2Trash==1.5.0,six==1.15.0,snowballstemmer==2.0.0,Sphinx @ git+https://github.com/jimmylai/sphinx.git@971540b26a8412399a9face3752cafc4519ff748,sphinx-rtd-theme==0.5.0,sphinxcontrib-applehelp==1.0.2,sphinxcontrib-devhelp==1.0.2,sphinxcontrib-htmlhelp==1.0.3,sphinxcontrib-jsmath==1.0.1,sphinxcontrib-qthelp==1.0.3,sphinxcontrib-serializinghtml==1.1.4,terminado==0.9.1,testpath==0.4.4,toml==0.10.2,tornado==6.1,tox==3.20.1,traitlets==5.0.5,typed-ast==1.4.1,typing-extensions==3.7.4.3,typing-inspect==0.6.0,urllib3==1.26.2,virtualenv==20.2.1,wcwidth==0.2.5,webencodings==0.5.1,widgetsnbextension==3.5.1,zipp==3.4.0
py37 run-test-pre: PYTHONHASHSEED='2358965140'
py37 run-test: commands[0] | python -m unittest fixit.tests.UseAssertInRule
...............
----------------------------------------------------------------------
Ran 15 tests in 0.135s

OK
______________________________________________________________________________________ summary ______________________________________________________________________________________
  py37: commands succeeded
  congratulations :)

Ran tox -e autofix

GLOB sdist-make: /Users/dkteo/Fixit/setup.py
autofix inst-nodeps: /Users/dkteo/Fixit/.tox/.tmp/package/1/fixit-0.1.2.zip
autofix installed: alabaster==0.7.12,appdirs==1.4.4,appnope==0.1.0,argon2-cffi==20.1.0,async-generator==1.10,attrs==20.3.0,Babel==2.9.0,backcall==0.2.0,black==20.8b1,bleach==3.2.1,certifi==2020.11.8,cffi==1.14.4,chardet==3.0.4,click==7.1.2,codecov==2.1.10,coverage==5.3,decorator==4.4.2,defusedxml==0.6.0,diff-cover==4.0.1,distlib==0.3.1,docutils==0.16,entrypoints==0.3,filelock==3.0.12,fixit @ file:///Users/dkteo/Fixit/.tox/.tmp/package/1/fixit-0.1.2.zip,flake8==3.8.4,idna==2.10,imagesize==1.2.0,importlib-metadata==3.1.0,inflect==5.0.2,ipykernel==5.3.4,ipython==7.19.0,ipython-genutils==0.2.0,ipywidgets==7.5.1,isort==5.6.4,jedi==0.17.2,Jinja2==2.11.2,jinja2-pluralize==0.3.0,jsonschema==3.2.0,jupyter==1.0.0,jupyter-client==6.1.7,jupyter-console==6.2.0,jupyter-core==4.7.0,jupyterlab-pygments==0.1.2,libcst==0.3.14,MarkupSafe==1.1.1,mccabe==0.6.1,mistune==0.8.4,mypy-extensions==0.4.3,nbclient==0.5.1,nbconvert==6.0.7,nbformat==5.0.8,nbsphinx==0.8.0,nest-asyncio==1.4.3,notebook==6.1.5,packaging==20.7,pandocfilters==1.4.3,parso==0.7.1,pathspec==0.8.1,pexpect==4.8.0,pickleshare==0.7.5,pluggy==0.13.1,prometheus-client==0.9.0,prompt-toolkit==3.0.8,psutil==5.7.3,ptyprocess==0.6.0,py==1.9.0,pycodestyle==2.6.0,pycparser==2.20,pyflakes==2.2.0,Pygments==2.7.2,pyparsing==2.4.7,pyre-check==0.0.41,pyre-extensions==0.0.19,pyrsistent==0.17.3,python-dateutil==2.8.1,pytz==2020.4,pywatchman==1.4.1,PyYAML==5.3.1,pyzmq==20.0.0,qtconsole==5.0.1,QtPy==1.9.0,regex==2020.11.13,requests==2.25.0,Send2Trash==1.5.0,six==1.15.0,snowballstemmer==2.0.0,Sphinx @ git+https://github.com/jimmylai/sphinx.git@971540b26a8412399a9face3752cafc4519ff748,sphinx-rtd-theme==0.5.0,sphinxcontrib-applehelp==1.0.2,sphinxcontrib-devhelp==1.0.2,sphinxcontrib-htmlhelp==1.0.3,sphinxcontrib-jsmath==1.0.1,sphinxcontrib-qthelp==1.0.3,sphinxcontrib-serializinghtml==1.1.4,terminado==0.9.1,testpath==0.4.4,toml==0.10.2,tornado==6.1,tox==3.20.1,traitlets==5.0.5,typed-ast==1.4.1,typing-extensions==3.7.4.3,typing-inspect==0.6.0,urllib3==1.26.2,virtualenv==20.2.1,wcwidth==0.2.5,webencodings==0.5.1,widgetsnbextension==3.5.1,zipp==3.4.0
autofix run-test-pre: PYTHONHASHSEED='4017664482'
autofix run-test: commands[0] | isort -q .
autofix run-test: commands[1] | black .
All done! ✨ 🍰 ✨
95 files left unchanged.
______________________________________________________________________________________ summary ______________________________________________________________________________________
  autofix: commands succeeded
  congratulations :)

dkteo · 2020-11-30T23:07:18Z

@jimmylai PTAL.
Builds seem to be failing due to lint errors:

ERROR: D:\a\Fixit\Fixit\fixit\rules\use_assert_in.py Imports are incorrectly sorted and/or formatted.
Skipped 2 files
ERROR: InvocationError for command 'D:\a\Fixit\Fixit\.tox\lint\Scripts\isort.EXE' --check-only . (exited with code 1)

But it works fine locally when I run isort manually.

dkteo · 2020-12-01T00:22:43Z

@mvismonte PTAL!

jimmylai · 2020-12-01T00:29:58Z

@jimmylai PTAL.
Builds seem to be failing due to lint errors:
ERROR: D:\a\Fixit\Fixit\fixit\rules\use_assert_in.py Imports are incorrectly sorted and/or formatted.
Skipped 2 files
ERROR: InvocationError for command 'D:\a\Fixit\Fixit\.tox\lint\Scripts\isort.EXE' --check-only . (exited with code 1)
But it works fine locally when I run isort manually.

You can fix the error by sorting the imports properly using command tox -e autofix.

dkteo · 2020-12-01T01:37:08Z

@jimmylai thanks that did the trick.

However, it looks like the builds are still failing b/c of a test in another rule: UseClsInClassmethodRule.

tox -e py38 -- fixit.tests.UseClsInClassmethodRule
FAIL: test_INVALID_4 (fixit.common.testing.UseClsInClassmethodRule)
----------------------------------------------------------------------
AssertionError: Auto-fix did not produce expected result.
Expected:
class foo:
    # Do not replace in nested scopes.
    @classmethod
    async def cm(cls, b):
        b = cls
        b = lambda _: cls.__name__
        def g():
            return cls.__name__

        # Same-named vars in sub-scopes should not be replaced.
        **b = [a for a in [1,2,3]]**
        def f(a):
            return a + 1

But found:
class foo:
    # Do not replace in nested scopes.
    @classmethod
    async def cm(cls, b):
        b = cls
        b = lambda _: cls.__name__
        def g():
            return cls.__name__

        # Same-named vars in sub-scopes should not be replaced.
        **b = [cls for a in [1,2,3]]**
        def f(a):
            return a + 1

Seems like autofix is replacing the first a in b = [a for a in [1,2,3]] to cls even though it shouldn't be.

jimmylai · 2020-12-01T03:27:44Z

@jimmylai thanks that did the trick.

However, it looks like the builds are still failing b/c of a test in another rule: UseClsInClassmethodRule.

tox -e py38 -- fixit.tests.UseClsInClassmethodRule
FAIL: test_INVALID_4 (fixit.common.testing.UseClsInClassmethodRule)
----------------------------------------------------------------------
AssertionError: Auto-fix did not produce expected result.
Expected:
class foo:
    # Do not replace in nested scopes.
    @classmethod
    async def cm(cls, b):
        b = cls
        b = lambda _: cls.__name__
        def g():
            return cls.__name__

        # Same-named vars in sub-scopes should not be replaced.
        **b = [a for a in [1,2,3]]**
        def f(a):
            return a + 1

But found:
class foo:
    # Do not replace in nested scopes.
    @classmethod
    async def cm(cls, b):
        b = cls
        b = lambda _: cls.__name__
        def g():
            return cls.__name__

        # Same-named vars in sub-scopes should not be replaced.
        **b = [cls for a in [1,2,3]]**
        def f(a):
            return a + 1

Seems like autofix is replacing the first a in b = [a for a in [1,2,3]] to cls even though it shouldn't be.

It's likely caused by a new change in new version of LibCST.
Instagram/LibCST#413

There is a fix that @zsol is working on Instagram/LibCST#423.

For now, to unblock this PR, we can pin LibCST version to the previous stable version 0.3.13.
@dkteo you can pin the version in https://github.com/Instagram/Fixit/blob/master/requirements.txt
(libcst==0.3.13)

thatch · 2020-12-01T05:18:42Z

fixit/rules/use_assert_in.py

+
+class UseAssertInRule(CstLintRule):
+    """
+    Discourages use of ``assertTrue(x in y)`` and ``assertFalse(x in y)`` as it is deprecated (https://docs.python.org/3.8/library/unittest.html#deprecated-aliases).


You can use reStructuredText here; please wrap to a reasonable line length too.

shortened the string length.

thatch · 2020-12-01T05:19:30Z

fixit/rules/use_assert_in.py

+    Use ``assertIn(x, y)`` and ``assertNotIn(x, y)``) instead.
+    """
+
+    ONCALL_SHORTNAME = "instagram_server_framework"


This isn't in the open-source version.

thanks for catching. removed.

thatch · 2020-12-01T05:21:33Z

fixit/rules/use_assert_in.py

+        if m.matches(
+            node,
+            m.Call(
+                func=m.Attribute(value=m.Name("self"), attr=m.Name("assertTrue")),


I don't know if this is based on some pre-existing code, but I think you can do this more effectively with a single extract; save whether you see the couple of places "not" could be, whether the method is assertTrue or assertFalse, and construct once with the xor of those 3 without a page of if statements.

makes sense.
this was taken from the internal rule though. i'll leave it for now so there's consistency between both places.

Yeah, use a single extract can be more effective and that can leave as future work. @dkteo can you add a # TODO: comment to the code as a note?
Also, once this is merged, we'll want to remove the internal copy to avoid duplication since this will be released to internal dev environment.

codecov-io · 2020-12-01T21:55:47Z

Codecov Report

Merging #159 (b1a7d68) into master (6e662b2) will increase coverage by 0.28%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #159      +/-   ##
==========================================
+ Coverage   84.71%   84.99%   +0.28%     
==========================================
  Files          86       88       +2     
  Lines        3532     3613      +81     
==========================================
+ Hits         2992     3071      +79     
- Misses        540      542       +2

Impacted Files	Coverage Δ
fixit/rules/use_assert_in.py	`100.00% <100.00%> (ø)`
fixit/common/testing.py	`92.30% <0.00%> (-0.96%)`	⬇️
fixit/rules/explicit_frozen_dataclass.py	`100.00% <0.00%> (ø)`
fixit/common/utils.py	`92.03% <0.00%> (+0.07%)`	⬆️
fixit/rules/import_constraints.py	`92.39% <0.00%> (+0.78%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e662b2...b1a7d68. Read the comment docs.

jimmylai

Good job! Overall LGTM. Just some comments.

jimmylai · 2020-12-01T23:14:40Z

requirements.txt

@@ -1,3 +1,3 @@
 flake8>=3.8.1
-libcst>=0.3.10
+libcst>=0.3.13


@Kronuz just released 0.3.15 today which should fix the issue.
@dkteo can you revert this change?

jimmylai · 2020-12-01T23:15:50Z

fixit/rules/use_assert_in.py

+        if m.matches(
+            node,
+            m.Call(
+                func=m.Attribute(value=m.Name("self"), attr=m.Name("assertTrue")),


Yeah, use a single extract can be more effective and that can leave as future work. @dkteo can you add a # TODO: comment to the code as a note?
Also, once this is merged, we'll want to remove the internal copy to avoid duplication since this will be released to internal dev environment.

dkteo · 2020-12-02T00:28:29Z

@jimmylai I've addressed the comments but am unable to merge b/c I don't have write access to the repo. Could you help me merge the PR? Thanks!

fixit/rules/use_assert_in.py

Co-authored-by: Jimmy Lai <[email protected]>

jimmylai · 2020-12-08T12:30:04Z

@dkteo, thanks for contributing!

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 30, 2020

dkteo force-pushed the dk/add_use_assert_in_rule branch 3 times, most recently from 96bdcb6 to da76a5e Compare November 30, 2020 23:01

dkteo force-pushed the dk/add_use_assert_in_rule branch from da76a5e to eb6baaa Compare December 1, 2020 01:06

thatch reviewed Dec 1, 2020

View reviewed changes

dkteo force-pushed the dk/add_use_assert_in_rule branch 3 times, most recently from 8ae4fde to 00558e1 Compare December 1, 2020 21:53

jimmylai approved these changes Dec 1, 2020

View reviewed changes

Add UseAssertIn lint rule to Fixit

0434d2c

dkteo force-pushed the dk/add_use_assert_in_rule branch from 00558e1 to 0434d2c Compare December 1, 2020 23:44

jimmylai reviewed Dec 3, 2020

View reviewed changes

fixit/rules/use_assert_in.py Outdated Show resolved Hide resolved

Update fixit/rules/use_assert_in.py

b1a7d68

Co-authored-by: Jimmy Lai <[email protected]>

jimmylai merged commit 068f875 into Instagram:master Dec 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add UseAssertIn lint rule to Fixit #159

Add UseAssertIn lint rule to Fixit #159

dkteo commented Nov 30, 2020 •

edited

Loading

dkteo commented Nov 30, 2020

dkteo commented Dec 1, 2020

jimmylai commented Dec 1, 2020

dkteo commented Dec 1, 2020 •

edited

Loading

jimmylai commented Dec 1, 2020

thatch Dec 1, 2020

dkteo Dec 1, 2020

thatch Dec 1, 2020

dkteo Dec 1, 2020

thatch Dec 1, 2020 •

edited

Loading

dkteo Dec 1, 2020

jimmylai Dec 1, 2020

codecov-io commented Dec 1, 2020 •

edited

Loading

jimmylai left a comment

jimmylai Dec 1, 2020

dkteo Dec 1, 2020

jimmylai Dec 1, 2020

dkteo commented Dec 2, 2020

jimmylai commented Dec 8, 2020

Add UseAssertIn lint rule to Fixit #159

Add UseAssertIn lint rule to Fixit #159

Conversation

dkteo commented Nov 30, 2020 • edited Loading

Summary

Test Plan

dkteo commented Nov 30, 2020

dkteo commented Dec 1, 2020

jimmylai commented Dec 1, 2020

dkteo commented Dec 1, 2020 • edited Loading

jimmylai commented Dec 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thatch Dec 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Dec 1, 2020 • edited Loading

Codecov Report

jimmylai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkteo commented Dec 2, 2020

jimmylai commented Dec 8, 2020

dkteo commented Nov 30, 2020 •

edited

Loading

dkteo commented Dec 1, 2020 •

edited

Loading

thatch Dec 1, 2020 •

edited

Loading

codecov-io commented Dec 1, 2020 •

edited

Loading