ast changes for "debug" f-strings
I added an optional "expr_text" field to the FormattedValue node, which represents the text of the expression in a "debug" f-string expression. So in f'{x=}', expr_text would be "x=". This strictly speaking isn't necessary. I could have added another Constant node for "x=" and left FormattedValue alone. I didn't for three reasons: it was expedient; it didn't require a lot of surgery to f-string parsing, which the extra Constant node would require; and it allowed the Python/ast_unparse.c code to produce a string that was more consistent with input string. Now that I have more time, I'm rethinking this decision. I'd rather not have the extra churn that modifying the ast node will cause to downstream code. Someone (Serhiy?) already pointed this out somewhere, but now I can't find the original message. But the issue of the fidelity of ast_unparse is still there. I think the only place this code is used is "from __future__ import annotations", but for all I know there's a plan to use it elsewhere. Does anyone care that f'{x=}' would become f'x={x!r}' if I removed expr_text from the FormattedValue node? This isn't the only place where ast_unparse isn't exact with what it produces. It's not designed to give the exact source text back, just something equivalent. But it's not clear to me how equivalent it needs to be. Even if I did add the extra Constant node, then you'd have the following discrepancy: f'value: {x=}' would produce: [Expr(value=JoinedStr(values=[Constant(value='value: ', kind=None), Constant(value='x=', kind=None), FormattedValue(value=Name(id='x', ctx=Load()), conversion=114, format_spec=None, expr_text='x=')]))] Note that there are two consecutive Constant nodes here. So this: f'value: x={x!r}' would produce the slightly different: [Expr(value=JoinedStr(values=[Constant(value='value: x=', kind=None), FormattedValue(value=Name(id='x', ctx=Load()), conversion=114, format_spec=None)]))] Note that there is a single Constant node, with the combined string in it. I'm not sure how much we care about all of this, but let me know if you have a strong feeling about it. My suggestion for the 3.8b1 release is to add the extra Constant node and remove expr_text from the FormattedValue node. This would at least mean that there's no change to the ast definitions due to this feature. If we decide that the two consecutive Constant nodes generated by f'value: {x=}' need to be combined, then we can do that post 3.8b1. But it's a lot more work to combine the nodes while the f-string is being compiled. Let me know your thoughts, since time is of the essence. Eric
This strictly speaking isn't necessary. I could have added another Constant node for "x=" and left FormattedValue alone. I didn't for three reasons: it was expedient; it didn't require a lot of surgery to f-string parsing, which the extra Constant node would require; and it allowed the Python/ast_unparse.c code to produce a string that was more consistent with input string.
Agreed.
Does anyone care that f'{x=}' would become f'x={x!r}' if I removed expr_text from the FormattedValue node?
Yes, when i was implementing f-string debugging support to Berker's astor project the roundtrip tests i wrote is failing because of it adds an extra `!r` to end. Then i realized you added a new field (expr_text) for that.
I'm not sure how much we care about all of this, but let me know if you have a strong feeling about it.
I don't think we should complicate this. The current version is very simple and understandable.
On 5/20/2019 10:32 AM, Batuhan Taskaya wrote:
This strictly speaking isn't necessary. I could have added another Constant node for "x=" and left FormattedValue alone. I didn't for three reasons: it was expedient; it didn't require a lot of surgery to f-string parsing, which the extra Constant node would require; and it allowed the Python/ast_unparse.c code to produce a string that was more consistent with input string.
Agreed.
Does anyone care that f'{x=}' would become f'x={x!r}' if I removed expr_text from the FormattedValue node?
Yes, when i was implementing f-string debugging support to Berker's astor project the roundtrip tests i wrote is failing because of it adds an extra `!r` to end. Then i realized you added a new field (expr_text) for that.
I'm not sure how much we care about all of this, but let me know if you have a strong feeling about it.
I don't think we should complicate this. The current version is very simple and understandable.
I think the salient question is: does the lack of expr_text make anything more difficult for anyone? As Serhiy said in the other email, lots of things are lost while round-tripping, this would just be another one. Anyone really interested in high-fidelity round trips already can't use the ast module to unparse from. Eric
20.05.19 16:25, Eric V. Smith пише:
I added an optional "expr_text" field to the FormattedValue node, which represents the text of the expression in a "debug" f-string expression. So in f'{x=}', expr_text would be "x=".
This strictly speaking isn't necessary. I could have added another Constant node for "x=" and left FormattedValue alone. I didn't for three reasons: it was expedient; it didn't require a lot of surgery to f-string parsing, which the extra Constant node would require; and it allowed the Python/ast_unparse.c code to produce a string that was more consistent with input string.
Now that I have more time, I'm rethinking this decision. I'd rather not have the extra churn that modifying the ast node will cause to downstream code. Someone (Serhiy?) already pointed this out somewhere, but now I can't find the original message.
Of course I am for removing "expr_text". This makes the AST simpler. I do not care about ast_unparse because it does nor roundtrip in general. For example it can add and remove parenthesis, commas, backslashes. Currently it even has a bug in handling some corner cases in f-strings -- nobody complains. In theory it is possible to detect when !d was used (parse the previous Constant value and compare the result with the FormattedValue expression). We can implement this if there is a need after beta1.
participants (3)
-
Batuhan Taskaya
-
Eric V. Smith
-
Serhiy Storchaka