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

Do not crash on call in except statement #187

Merged
merged 1 commit into from Sep 28, 2021
Merged

Conversation

jaap3
Copy link
Contributor

@jaap3 jaap3 commented Sep 28, 2021

This fixes the crash reported in issue #171. Not sure if this is the correct way to fix this, but at least it doesn't crash.

@jaap3 jaap3 marked this pull request as ready for review September 28, 2021 20:21
Copy link
Collaborator

@cooperlees cooperlees left a comment

Choose a reason for hiding this comment

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

Looks right to me. If CI passes I'll merge and look at getting a version out.

Many thanks for the Test driven fix! Welcome to flake8-bugbear family of contributors.

Copy link
Collaborator

@cooperlees cooperlees left a comment

Choose a reason for hiding this comment

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

Bah - Sorry to be a pain, but our good friend black is uphappy with the formatting. Can you just fix that.

@jaap3
Copy link
Contributor Author

jaap3 commented Sep 28, 2021

Super weird, I already ran black on it. Just ran it again and I get no changes:

$ black .
All done! ✨ 🍰 ✨
25 files left unchanged.
$ black --version
black, version 21.9b0

@cooperlees
Copy link
Collaborator

Super weird, I already ran black on it. Just ran it again and I get no changes:

$ black .
All done! ✨ 🍰 ✨
25 files left unchanged.
$ black --version
black, version 21.9b0

We're running and testing out the "experimental-string-processing" feature.

So you'll need: black --experimental-string-processing .

@jaap3
Copy link
Contributor Author

jaap3 commented Sep 28, 2021

Ah, just noticed that as well. Should be good now.

Edit: Oof, I don't like what black did there :(. Why isn't the test right above it reformatted? I basically copied the multiline string from there.

# akin to test_does_not_crash_on_tuple_expansion_in_except_statement
# see https://github.com/PyCQA/flake8-bugbear/issues/171
syntax_tree = ast.parse(
"foo = lambda: IOError\ntry:\n ...\nexcept (foo(),):\n ...\n"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I too, don't like this autoformat ...

cc: @ichard26 @JelleZijlstra @zsol - I think this will get complaints ... thoughts?

Copy link
Contributor

@ichard26 ichard26 Sep 28, 2021

Choose a reason for hiding this comment

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

Aren't user splits preserved unless they violate the length limit according to the ESP rules? Or does a function call mess that up?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe the issue is that the string put together isn't long enough to hit the line length limit, so we put it all together.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is similar to psf/black#1540

@cooperlees cooperlees merged commit fdfa3a0 into PyCQA:master Sep 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants