Skip to content

bpo-45019: Cleanup module freezing and deepfreeze #29772

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

Merged
merged 2 commits into from
Nov 26, 2021

Conversation

kumaraditya303
Copy link
Contributor

@kumaraditya303 kumaraditya303 commented Nov 25, 2021

@kumaraditya303 kumaraditya303 marked this pull request as ready for review November 25, 2021 11:36
@kumaraditya303 kumaraditya303 requested a review from a team as a code owner November 25, 2021 11:36
@kumaraditya303
Copy link
Contributor Author

cc @gvanrossum

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Without looking carefully yet, I think I like your cleanup of freeze_modules.py, but not the rename of the deepfreeze directory. Can you undo the latter and just focus on improving the script?

@kumaraditya303
Copy link
Contributor Author

kumaraditya303 commented Nov 25, 2021

Without looking carefully yet, I think I like your cleanup of freeze_modules.py, but not the rename of the deepfreeze directory. Can you undo the latter and just focus on improving the script?

EDIT: @gvanrossum done

@kumaraditya303 kumaraditya303 requested review from gvanrossum and removed request for a team November 25, 2021 16:53
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

This is great. I have a few nits.

I also wonder if there aren't more improvements we can make, e.g. use relative paths throughout the code, rather than computing absolute paths and then making them relative for the generated code again.

Oh, and it would be nice to be able to freeze some submodules in a package but not all of them -- e.g. I'd like to freeze encodings/{__init__,utf_8,aliases}.py but not the remaining 100 submodules in that package.

@kumaraditya303
Copy link
Contributor Author

kumaraditya303 commented Nov 26, 2021

Freeze encoding here #29788

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Looks good for this phase! I'll merge this now. Then you can work on e.g. changes groups of functions to classes and partially freezing encodings.

@bedevere-bot
Copy link

@gvanrossum: Please replace # with GH- in the commit message next time. Thanks!

@kumaraditya303
Copy link
Contributor Author

kumaraditya303 commented Nov 26, 2021

Thanks @gvanrossum This was my first contribution to cpython, earlier was just a typo fix, glad that you merged it :).

@kumaraditya303 kumaraditya303 deleted the freeze branch November 26, 2021 16:53
@gvanrossum
Copy link
Member

Thanks Kumar for cleaning up our mess! :-) I am looking forward to seeing more from you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants