typeform: remove a regex call, rearrange checks#21459
typeform: remove a regex call, rearrange checks#21459JelleZijlstra merged 5 commits intopython:masterfrom
Conversation
This comment has been minimized.
This comment has been minimized.
JelleZijlstra
changed the title
for more information, see https://pre-commit.ci
hauntsaninja
left a comment
hauntsaninja
left a comment
There was a problem hiding this comment.
Thanks! Good chance to see if the benchmarking tech in sterliakov/mypy-issues#297 measures something :-)
| elif ( | ||
| str_value == "" | ||
| or str_value.isspace() | ||
| or (len(str_value) == 1 and str_value in ".,/:*-=[]\\") |
There was a problem hiding this comment.
where does the need for this change come from?
There was a problem hiding this comment.
According to Codex it accounted for a few % of all strings in mypy.
There was a problem hiding this comment.
The general theory is that it's good to be simple len/containment checks that mypyc optimizes, so we can avoid doing the slower regex call.
ilevkivskyi
left a comment
ilevkivskyi
left a comment
There was a problem hiding this comment.
Let's try this! I have just one question
| elif ( | ||
| str_value == "" | ||
| or str_value.isspace() | ||
| or (len(str_value) == 1 and str_value in ".,/:*-=[]\\") |
There was a problem hiding this comment.
Thinking a bit more about this, since we already check for identifier above, are there 1-char strings that are representing a valid Python type? It seems to me we can simply skip all 1-char strings.
There was a problem hiding this comment.
Good point. I think the shortest possible non-identifier valid type expression is actually 3 characters (a.b or a|b), so we can throw out all short strings here.
There was a problem hiding this comment.
Actually two-character strings aren't safe because something like T is valid. But len(str_value) < 2 should be safe.
This comment has been minimized.
This comment has been minimized.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
3 participants
Related to #21262 (comment) : I haven't benchmarked but this is likely to be faster.