New violation: Prefer TypeError for unexpected types #34

swellander · 2021-10-22T05:03:50Z

This currently only covers the one example case outlined by @sbrugman in issue #32. I want to make sure that I'm on the right track. Thanks!

sbrugman · 2021-10-23T10:21:55Z

Seems like you're on the right track, great job so far!. (w.r.t. specific tryceratops conventions @guilatrova should review).

Assuming that the base case works well, we should ensure that the approach is conservative enough not to raise false positives.

Cases containing elif are not completely covered: all clauses should be taken into account to prevent false positives. All conditions should be checking types (e.g. isinstance).
The second parameter of isinstance can be a single type, but also a tuple or union of types. The current code seems to work just fine with that, but is not testing it.

There are also a couple of opportunities to increase the recall of the approach:

Next to isinstance, there are other functions to check types in python (at least issubclass and callable). The same approach should work for them.
Similarly we can consider these exceptions beyond ValueError: Exception and RuntimeError.
Negations (e.g. not isinstance(a, int)) could be included. Note that there are more conditions for which this violation would be appropriate, however they conditions (and the testing for them) would be of increasing complexity. I'd argue that the negation is common enough, and of little overhead, to include.

For all of them test cases should be included (especially testing potential edge cases causing false positive).

src/tryceratops/analyzers/conditional.py

docs/violations/TC004.md

src/tests/samples/violations/call_prefer_type_error.py

guilatrova · 2021-10-26T15:41:40Z

Thanks @swellander and @sbrugman for the support.

I believe there are still some improvements to do as mentioned by @sbrugman , but I'm glad to see we're moving forward!

I can also contribute more if needed, let me know how's your time! :)

swellander · 2021-10-29T00:22:24Z

@sbrugman @guilatrova Thank you for the helpful reviews! You both raise some good points 😉 I'm already learning a lot. I have some time now to go over these comments and will try to have at least some revisions posted later tonight (PST).

swellander · 2021-10-29T03:44:45Z

src/tests/samples/violations/conditional_prefer_type_error.py

+def incorrect_multiple_operators(some_arg):
+    if (False and (3 == 3)) or (not isinstance(some_arg, int) and True):
+        pass
+    else:
+        raise Exception("...")  # should be typeerror


I added this case but am actually curious whether or not you all think it should be included.

I guess that will be tough to ensure we understood the developer's intention. (We might get it sometimes, and miss other times).

I'd prefer to remove it.
Thoughts? @sbrugman

Boolean functions including isinstance are endless and not guaranteed to be exclusively type checking. I'd opt for restricting ourselves to the clear-cut cases.

Yes, strict cleat-cut cases like:

if isinstance()

if not isinstance()

if isinstance() and isinstance()

if isinstance() and not isinstance()

if isinstance() and not isinstance() and isinstance()

...

(and variations you did e.g. callable, issubclass, etc...)

WDYT @swellander ?

That makes sense to me 👍

src/tryceratops/analyzers/conditional.py

swellander · 2021-10-29T03:50:01Z

Ok I think i've addressed both of your comments. Lmk if I missed anything.

guilatrova · 2021-11-01T13:25:40Z

src/tests/samples/violations/conditional_prefer_type_error.py

+        raise Exception("...")  # should be typeerror
+
+
+def incorrect_elif(some_arg):


This is not as obvious to me. I'd suggest to restricting where all conditions consist of one of the type checking functions

After reviewing it again, I agree with @sbrugman

It's hard to tell the exception being raised there was caused by the type checking.

src/tryceratops/analyzers/conditional.py

guilatrova · 2021-11-01T13:31:52Z

We're in great shape here! Feel free to convert from a draft to a real PR.

Great work, I'm impressed by:

How clear your code is
Great commit history (they'll be pretty on the changelog)
Following the conventions and organization followed by the project

Lastly, great work on rebasing your git history without I even asking. You're clearly a great developer! 👏
I'm glad that I had your help to push forward this nice feature to keep python try/except pretty ✨

sbrugman · 2021-11-01T18:14:52Z

src/tests/samples/violations/conditional_prefer_type_error.py

+
+
+def incorrect_ArithmeticError(some_arg):
+    if isinstance(some_arg, int):


Perhaps parametrize all these tests using the exception types for simplicity

guilatrova

I run it on some private code I have here, and I'm surprised it catch some violations that I implemented haha (which is good!).

There was one case that failed though (fake alert), let me share so we can start discussing:

Case 1 (failing)

if val is not None and (
    not isinstance(val, int) or val < 0
):
    raise ValueError(...)  # <- Got alert

I think for this case we can't mark errors because the condition is complex and contains validation unrelated to type.

Which is in sync with out discussion: #34 (comment)

Also, let's have a unit test to validate we ignore this scenario.

Thoughts? @swellander

Case 2 (Worked, but we should have a test)

Example 1: (We should support)

if isinstance(val1, type1) and isinstance(val2, type2):
    ...
else:
    raise ValueError("...")

Example 2: (It would be nice to support, I'm ok if we skip it for now)

if not (isinstance(sources, list) and
                all(isinstance(v, str) for v in sources)):
            raise AssertionError("'sources' must be a list of strings")

If ALL conditions for an if are checking the type, then we mark as violation!

Remove a useless `else`

As mentioned here: #34 (comment)

Great work so far! Let me know if you have the bandwidth to finish the implementation as suggested above @swellander

guilatrova · 2021-11-15T14:34:17Z

@swellander How's your time? Do you think you will be able to make the final adjustments?

swellander · 2021-11-17T04:23:27Z

@guilatrova yes, sorry again for the delay! I will have time to submit work tomorrow night if that works for you. If not, I don't want to hold up the project

src/tests/samples/violations/conditional_prefer_type_error.py

swellander · 2021-11-18T06:48:50Z

@guilatrova @sbrugman I think I've addressed all your comments except for the parametrization of tests and the Case 2, Example 2 from your review, @guilatrova. I hope to get to those two changes tomorrow night. Thanks!

P.S. I pushed these latest changes under their own commit for simplicity, but will squash them together with the others once approved.

swellander · 2021-11-19T05:40:00Z

src/tests/analyzers_conditional_test.py

+    for non_type_exception in STANDARD_NON_TYPE_ERROR_IDS:
+        with tempfile.NamedTemporaryFile(mode="r+") as tmp:
+            file_content = (
+                "if isinstance(some_arg, int):\n"
+                "   pass\n"
+                "else:\n"
+                f'   raise {non_type_exception}("...")'
+            )
+            tmp.write(file_content)
+            tmp.seek(0)
+            content = tmp.read()
+
+            tmp_tree = ast.parse(content)
+            tmp_violations = analyzer.check(tmp_tree, "filename")
+
+            assert len(tmp_violations) == 1
+            assert_type_error(4, 9, tmp_violations[0])


@sbrugman is this what you had in mind when you mentioned parametrizing these tests?

I think he meant to try pytest.mark.parametrize. But I'm not confident it would make the code any easier.

Ah I see, thanks

guilatrova · 2021-11-19T13:32:10Z

@guilatrova @sbrugman I think I've addressed all your comments except for the parametrization of tests and the Case 2, Example 2 from your review, @guilatrova. I hope to get to those two changes tomorrow night. Thanks!

@swellander
I'd recommend the parametrization of tests and case 2 to be done later in future PRs, to keep this one shorter.
WDYT? Also, I'm excited to merge this feature, so let me know your thoughts.

Once the commits are "done", then I'll run on some private repos to sanity check.

guilatrova · 2021-11-19T21:54:34Z

Versioninig question

@sbrugman @swellander
Another important thought, should we consider this release a "breaking change"?

I'm thinking it should. Mostly because anyone that was using the 0.6.x version and tryceratops was passing, maybe now will start having problems. I'd feel more comfortable making it a version 1.x now.

Suggestion

If you agree, @swellander do you mind making your commit feat: add prefer TypeError analyzer a breaking change by adding the !? (e.g. feat!: add prefer TypeError analyzer).

swellander · 2021-11-21T21:12:43Z

I'd recommend the parametrization of tests and case 2 to be done later in future PRs, to keep this one shorter.
WDYT? Also, I'm excited to merge this feature, so let me know your thoughts.
@guilatrova saving those changes for a new PR sounds good 👍

And treating this feature as a breaking change makes sense to me! I'll use the breaking change syntax when I squash the commits together here shortly.

guilatrova · 2021-11-22T10:00:13Z

I just tested here against some real code and it looks working.

guilatrova · 2021-11-22T10:22:44Z

I don't have enough words to express my gratitude.

Thank you @sbrugman and @swellander for helping the community and for contributing to making everyone's Python code better ❤️ ✨

swellander · 2021-11-22T18:58:32Z

Thank you @sbrugman for the wise input and thank you @guilatrova for the helpful feedback and your positive, welcoming energy 🌞 !

swellander marked this pull request as draft October 22, 2021 05:16

guilatrova linked an issue Oct 26, 2021 that may be closed by this pull request

New violation: Prefer TypeError for unexpected types #32

Closed

guilatrova requested changes Oct 26, 2021

View reviewed changes

docs: add TypeError violation

b8daf6e

swellander force-pushed the feat/prefer-type-error branch 2 times, most recently from 9bb112a to 906c154 Compare October 29, 2021 03:35

swellander commented Oct 29, 2021

View reviewed changes

src/tryceratops/analyzers/conditional.py Show resolved Hide resolved

swellander force-pushed the feat/prefer-type-error branch from 906c154 to f91acb6 Compare October 29, 2021 14:47

guilatrova reviewed Nov 1, 2021

View reviewed changes

src/tryceratops/analyzers/conditional.py Outdated Show resolved Hide resolved

sbrugman reviewed Nov 1, 2021

View reviewed changes

guilatrova requested changes Nov 13, 2021

View reviewed changes

swellander marked this pull request as ready for review November 18, 2021 06:27

swellander commented Nov 18, 2021

View reviewed changes

src/tests/samples/violations/conditional_prefer_type_error.py Show resolved Hide resolved

swellander commented Nov 18, 2021

View reviewed changes

src/tests/samples/violations/conditional_prefer_type_error.py Show resolved Hide resolved

swellander commented Nov 18, 2021

View reviewed changes

src/tests/samples/violations/conditional_prefer_type_error.py Show resolved Hide resolved

swellander commented Nov 19, 2021

View reviewed changes

guilatrova mentioned this pull request Nov 19, 2021

Allow Click 8 #39

Merged

test: prefer TypeError analyzer

f1b6280

swellander force-pushed the feat/prefer-type-error branch from 09dc984 to 4bf53dd Compare November 21, 2021 21:21

feat!: add prefer TypeError analyzer

d9f8116

swellander force-pushed the feat/prefer-type-error branch from 4bf53dd to d9f8116 Compare November 21, 2021 21:22

guilatrova merged commit ac75e37 into guilatrova:main Nov 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New violation: Prefer TypeError for unexpected types #34

New violation: Prefer TypeError for unexpected types #34

swellander commented Oct 22, 2021

sbrugman commented Oct 23, 2021 •

edited

guilatrova commented Oct 26, 2021

swellander commented Oct 29, 2021

swellander Oct 29, 2021

guilatrova Nov 1, 2021

sbrugman Nov 1, 2021 •

edited

guilatrova Nov 13, 2021

swellander Nov 18, 2021

swellander commented Oct 29, 2021

guilatrova Nov 1, 2021

sbrugman Nov 1, 2021

guilatrova Nov 13, 2021

swellander Nov 18, 2021

guilatrova commented Nov 1, 2021

sbrugman Nov 1, 2021

guilatrova left a comment •

edited

guilatrova commented Nov 15, 2021

swellander commented Nov 17, 2021

swellander commented Nov 18, 2021 •

edited

swellander Nov 19, 2021

guilatrova Nov 19, 2021

swellander Nov 21, 2021

guilatrova commented Nov 19, 2021

guilatrova commented Nov 19, 2021

swellander commented Nov 21, 2021

guilatrova commented Nov 22, 2021

guilatrova commented Nov 22, 2021

swellander commented Nov 22, 2021

		raise Exception("...") # should be typeerror


		def incorrect_elif(some_arg):



		def incorrect_ArithmeticError(some_arg):
		if isinstance(some_arg, int):

New violation: Prefer TypeError for unexpected types #34

New violation: Prefer TypeError for unexpected types #34

Conversation

swellander commented Oct 22, 2021

sbrugman commented Oct 23, 2021 • edited

guilatrova commented Oct 26, 2021

swellander commented Oct 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbrugman Nov 1, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swellander commented Oct 29, 2021

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

guilatrova commented Nov 1, 2021

Choose a reason for hiding this comment

guilatrova left a comment • edited

Choose a reason for hiding this comment

Case 1 (failing)

Case 2 (Worked, but we should have a test)

Remove a useless else

guilatrova commented Nov 15, 2021

swellander commented Nov 17, 2021

swellander commented Nov 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guilatrova commented Nov 19, 2021

guilatrova commented Nov 19, 2021

Versioninig question

Suggestion

swellander commented Nov 21, 2021

guilatrova commented Nov 22, 2021

guilatrova commented Nov 22, 2021

swellander commented Nov 22, 2021

sbrugman commented Oct 23, 2021 •

edited

sbrugman Nov 1, 2021 •

edited

guilatrova left a comment •

edited

Remove a useless `else`

swellander commented Nov 18, 2021 •

edited