Skip to content

Comments

Make changes requested by Steering Council#57

Merged
lysnikolaou merged 5 commits intolysnikolaou:tstringsfrom
Wingysam:tstrings-updates
Apr 9, 2025
Merged

Make changes requested by Steering Council#57
lysnikolaou merged 5 commits intolysnikolaou:tstringsfrom
Wingysam:tstrings-updates

Conversation

@Wingysam
Copy link

@Wingysam Wingysam commented Apr 7, 2025

Of the four changes requested by the Steering Council, two of them are technical. I have made an attempt to make these two changes.

  • In the Interpolation object, expr is now called expression and conv is now called conversion.
  • The templatelib library has been moved under the string library.

I did have one question. There are some variables in the template object like expreq that are not the parameters/member names. Should these also be renamed to expressioneq & co?

@davepeck
Copy link

davepeck commented Apr 7, 2025

Does pycore_global_strings.h need to be updated too, to add STRUCT_FOR_ID(expression) and STRUCT_FOR_ID(conversion)?

@Wingysam
Copy link
Author

Wingysam commented Apr 7, 2025

You're right, it does. I'll fix that!

@Wingysam
Copy link
Author

Wingysam commented Apr 7, 2025

Interesting. My build locally works, I'll try a git clean and see if it fails.

@davepeck
Copy link

davepeck commented Apr 7, 2025

Interesting. My build locally works, I'll try a git clean and see if it fails.

(I deleted that comment -- still suspect it might be a me thing. Thanks for looking, though!)

@Wingysam
Copy link
Author

Wingysam commented Apr 7, 2025

Huh that's strange. Let me know if you get it to build or not!

Here's how it looks on my machine:

image

@davepeck
Copy link

davepeck commented Apr 7, 2025

Yeah, it builds and import string works fine in the cpython devcontainer.

Something about creating a docker image from it, though, is where things are going wrong. string is not included. Still digging. Thanks for the quick response!

@davepeck
Copy link

davepeck commented Apr 7, 2025

Ah, perhaps string needs to be added to LIBSUBDIRS in Makefile.pre.in.

@Wingysam
Copy link
Author

Wingysam commented Apr 7, 2025

I'm also able to build it in a docker container:
image

I'll try adding it to LIBSUBDIRS, let me know if that fixes it for you!

@davepeck
Copy link

davepeck commented Apr 7, 2025

Yup, that did the trick; the official docker-library/python Dockerfiles now result in working builds when aimed at your branch.

I'll update the pep750-examples repository's dev container to use this build.

Thanks again!

@davepeck
Copy link

davepeck commented Apr 7, 2025

One more detail:

Screenshot 2025-04-07 at 2 30 21 PM

That should probably report type(x) as <class 'string.templatelib.Template'>? (Here, and related for TemplateIter and Interpolation.)

@Wingysam
Copy link
Author

Wingysam commented Apr 7, 2025

Good catch, I've updated the types.

@davepeck davepeck mentioned this pull request Apr 8, 2025
Copy link
Owner

@lysnikolaou lysnikolaou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks @Wingysam!

@lysnikolaou lysnikolaou merged commit 77822be into lysnikolaou:tstrings Apr 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants