Add smoke tests for bytes literals and recursive f-strings #134
Conversation
PeterJCLaw
force-pushed
the
more-bytes-string-coverage
branch
2 times, most recently
from
97df274
to
1326938
Compare
|
Somewhat baffled by the coverage apparently decreasing here! |
| 'x = (f"Wobble {f"{func(kwarg=f"{boo!r}")}"!r}.",)', | ||
| ) | ||
|
|
||
| self.assertEqual(m.view_nodes_at(1, 6), { |
There was a problem hiding this comment.
Can you also check one or both of the inner f-strings?
There was a problem hiding this comment.
1c42c83 does this to an extent. We don't seem to get the full details of the inner nodes though. I'm not sure if that's expected.
There was a problem hiding this comment.
Ah, that's sort of expected pre-3.12 but it's also sort of a bug in 3.12 where getting the tokens becomes possible. ASTText can deal with f-strings, but the package is called asttokens...so if you're willing to look into this, search for include_joined_str.
There was a problem hiding this comment.
Yeah, I'd seen that argument to the iter_children_* functions. That's the reason for using node.values over the view_nodes_at approach in the below assertion. Perhaps I'm misunderstanding, but I think the lack of text is coming from the calls to ASTTokens.get_text_positions where the implementations don't return useful values for f-strings (due to not having a first_token). Checking ASTText.get_text_positions's behaviour might be useful, though I'm assuming that that's covered elsewhere.
I'm not sure what you're suggesting be investigated/potentially changed here?
There was a problem hiding this comment.
I'm suggesting that in 3.12+, MarkTokens should start attaching first_token to inner f-string nodes.
There was a problem hiding this comment.
Ah, I see. That feels worthwhile, but perhaps a bit out of the scope of this PR? My main goal here was to add some lightweight coverage for things that #132 seemed to show weren't covered by unit tests.
Having a very quick look, just adding include_joined_str=True in a few places doesn't seem to pass the tests, so this would probably need a deeper dive really.
There was a problem hiding this comment.
Right, please just add a comment explaining the situation with a TODO, because the assertion is indeed hard to understand.
There was a problem hiding this comment.
Is 736d4e3 sufficiently clear?
Coverage of the related codepaths was low within the main unit-test suite, which this aims to help with. These tests are admittedly not great, however should catch any glaring issues.
This demonstrates what (little) information we do get about the child nodes within the JoinedStr.
PeterJCLaw
force-pushed
the
more-bytes-string-coverage
branch
from
d056553
to
1c42c83
Compare
|
Thanks again for all your contributions! |
Coverage of the related codepaths was low within the main unit-test suite, which this aims to help with. These tests are admittedly not great, however should catch any glaring issues.