-
-
Notifications
You must be signed in to change notification settings - Fork 306
Add basic support for TemplateStr #2793
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great ! And +1 for basic support now rather than perfect support too late for 3.14 release
| self, | ||
| *, | ||
| value: NodeNG, | ||
| str: str, # pylint: disable=redefined-builtin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Pierre-Sassoulas Should we change the attribute name here? str is the one used by the ast node but it's a bit unfortunate in my opinion since it shadows the builtin str.
Not sure using an alias here would be better though.
https://docs.python.org/3.14/library/ast.html#ast.Interpolation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, not ideal, but what should we use instead ? Any non adherence to ast's API is mental load on astroid's user and documentation burden on us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wanted to discuss it first. I also think that str while not ideal is still the best option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before you merged I was writing: Let's wait to read other opinions. Some maintainers might still be ambitious enough to keep astroid a wrapper above the AST and its multiple interpreters :D
It's still possible to change this later if someone want, we're in alpha/betas atm. Being able to test this in pylint fast is good too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about that. Guess I was too fast this time 😅
Yeah, we can still change it. Furthermore I think the biggest issue is actually in the postinit arg name as that's used on it's own. Accessing the attribute itself shouldn't really be an issue with node.str.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2793 +/- ##
==========================================
+ Coverage 93.26% 93.29% +0.03%
==========================================
Files 91 91
Lines 11009 11068 +59
==========================================
+ Hits 10267 10326 +59
Misses 742 742
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
Do you want me to release an astroid beta ? |
I still need to debug at least one astroid issue for 3.14. Might make sense to wait a bit. |
Looks like what I was seeing is an existing issue with astroid not being able to infer Feel free to do a new beta release if you've some time. |
|
I can't find the conversation about it but I remember saying somewhere that this kind of call should be uninferable (you don't know at static analysis time what is the value for the interpreter or the os, or...). If we want to do better, we would need to use a different constant for each, maybe we can discuss this in 113. |
Description
Add the basic buildings blocks for
ast.TemplateStrandast.Interpolationnodes. Most of it is pretty similar to the f-string nodes.The implementation isn't complete but good enough to not outright fail if a t-string is used anywhere in a file. Future PRs should look to address the following
as_string()logic. The basics work but there might be a lot of edge case which need to be handled properly. The existing f-string tests could provide a good starting point.astroid/tests/test_nodes.py
Lines 295 to 307 in f9e109e
JoinedStr._infer.astroid/astroid/nodes/node_classes.py
Lines 4775 to 4781 in f9e109e
--
https://docs.python.org/3.14/library/ast.html#ast.TemplateStr
Ref #2789