Adding more f-string features #4827
Conversation
|
@hediet Would love to see this issue resolved. |
|
Raymond can you please reach out internally to find someone to approve this? |
aiday-mar
left a comment
aiday-mar
left a comment
There was a problem hiding this comment.
Hi, thanks for the PR. I am not the owner of the area, so I have a few questions I wanted to ask you to gain a better understanding of the PR. Thank you.
| ], | ||
| fStringBody: [ | ||
| fStringBody3: [ | ||
| [/[^\\'\{\}]+/, 'string'], |
There was a problem hiding this comment.
I have several questions about the PR which I would like to ask to better understand this PR.
How come here on line 262 it's written
[/[^\\'\{\}]+/, 'string']
But on line 273 we have:
[/[^\\'\{\}]+$/, 'string', '@popall'],
[/[^\\'\{\}]+/, 'string'],
For the other case? It seems like this code is not the exact duplicate of the other code, as it does not contain the third value '@popall'.
Also would you know what popall here refers to? I am trying to understand.
| [/\{f"{1}/, 'string', '@fDblStringBody1'], // for nested f-strings | ||
| [/\{[^\}'":!=]+/, 'identifier', '@fStringDetail'], | ||
| [/\\./, 'string'], | ||
| [/'''/, 'string.escape', '@popall'], |
There was a problem hiding this comment.
Why was this added? The equivalent of this does not seem to be present in the fStringBody1 array. What use case does it fix?
| [/\{[^\}'":!=]+/, 'identifier', '@fStringDetail'], | ||
| [/\\./, 'string'], | ||
| [/"""/, 'string.escape', '@popall'], | ||
| [/"/, 'string.escape'], |
There was a problem hiding this comment.
So we escape on the " and on the """. What use case does it fix? And why does line 298 not have @popall as third value in the array?
| [/:[^}]+/, 'string'], // for numeric formatting | ||
| [/[!][ars]/, 'string'], // only !a, !r, !s are supported by f-strings: https://docs.python.org/3/tutorial/inputoutput.html#formatted-string-literals | ||
| [/=/, 'string'], | ||
| [/=/, 'identifier'], |
There was a problem hiding this comment.
Why was this changed from string to identifier?
| [/=/, 'string'], | ||
| [/=/, 'identifier'], | ||
| [/'[^\']+'/, 'string'], | ||
| [/"[^\""]+"/, 'string'], |
There was a problem hiding this comment.
Should this be instead
/"[^\"]+"/
?
| ], | ||
| fStringDetail: [ | ||
| [/[:][^}]+/, 'string'], | ||
| [/:=[^}]+/, 'identifier'], // walrus operator |
There was a problem hiding this comment.
Can you give me examples of where these would activate? Why are they followed by 'identifier' and not string?
|
Looking forward to this fix! |
|
Would love to see this PR merged! |
|
Hi everyone thanks for the comments. I left some questions regarding the PR which I'd like to enquire on before merging the PR. If there is no response, I may close the PR in the future to clean the PR stack. |
aiday-mar
added
the
~info-needed
aiday-mar
changed the title
f-string features
|
Hi, We at a lot of request for this feature at windmill.dev since we use a monaco-based editor. We have some amount of expertise in Monaco and are happy to give a hand at review or improvement. Is it needed or helpful at all? |
|
Hi @rubenfiszel thanks for suggesting. As I am not the direct owner of this area, I asked several questions regarding the PR. I would find it helpful if you or the author responds to them. |
|
Would be great to see this fix come to fruition, or someone else take up the task. Been experiencing this issue for some time. |
|
Any updates on this? |
|
Hi as mentioned, I have asked questions on the PR and am awaiting the responses. |
|
@samstrohkorbatt Any chance we could convince you to come back to this? Seems like you are very close to the final solution. If you are finding time too tight, would it be alright for another member to take over the pr? |
|
Hi as mentioned the original author of the PR has not provided responses to my feedback. It looks like this PR is not advancing further so it will be closed. |
10 participants
Fixed issue #4601 and add more f-string features with tests: