Skip to content

Conversation

@vsajip
Copy link
Contributor

@vsajip vsajip commented Nov 10, 2022

… to help the user pinpoint the problem.

@mhils
Copy link
Member

mhils commented Nov 10, 2022

Do you have an example where this is relevant?

@vsajip
Copy link
Contributor Author

vsajip commented Nov 10, 2022

I had an extra newline after Args: in a docstring and it failed the assertion "not starting with '\n'", but that didn't tell me which docstring it was. I had to change the source to catch the AssertionError to see the location of my error.

@mhils
Copy link
Member

mhils commented Nov 10, 2022

Thanks for clarifying! We don't want to crash in this case, we want to handle the whitespace gracefully. #459 fixes the underlying issue.

@vsajip
Copy link
Contributor Author

vsajip commented Nov 10, 2022

Yes, agreed, of course - this is better. I didn't know how strict you wanted to be, and didn't want to presume you'd be OK with relaxing the condition.

@mhils
Copy link
Member

mhils commented Nov 10, 2022

All good, thanks. :) As a general rule, docstring conversion should never crash.

@mhils mhils closed this Nov 10, 2022
@vsajip
Copy link
Contributor Author

vsajip commented Nov 10, 2022

What about the "\t" not in contents assertion, though? Couldn't a stray tab char in the source cause a crash?

@mhils
Copy link
Member

mhils commented Nov 10, 2022

Yes, that's a very fair point. We probably just want to remove that line? Do you want to send a PR? :)

@vsajip
Copy link
Contributor Author

vsajip commented Nov 10, 2022

Also, the assertions are there to catch unexpected conditions (unconsidered corner case, or regression) - if they are ever going to fire, the error message may as well be more informative than at present.

@mhils mhils reopened this Nov 10, 2022
@mhils
Copy link
Member

mhils commented Nov 10, 2022

Fair point - let's pipe out the docstring in question, but let's not allocate an unneeded error message. :)

@mhils mhils enabled auto-merge (squash) November 10, 2022 16:19
@vsajip
Copy link
Contributor Author

vsajip commented Nov 10, 2022

Sure, that's fine by me 😄

@mhils mhils merged commit 6df629d into mitmproxy:main Nov 10, 2022
@vsajip vsajip deleted the assertion-message branch November 10, 2022 21:55
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.

2 participants