Skip to content
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

Refactor to use C++ implementation of requirement gathering for Python. #3538

Merged
merged 1 commit into from
Feb 21, 2025

Conversation

dom96
Copy link
Collaborator

@dom96 dom96 commented Feb 13, 2025

The current state of things is that the logic for gathering package requirements is duplicated in TS and C++. This PR moves it into workerd so that the same implementation can be used in workerd and EW.

Copy link

github-actions bot commented Feb 13, 2025

The generated output of @cloudflare/workers-types matches the snapshot in types/generated-snapshot 🎉

@a-robinson a-robinson removed their request for review February 13, 2025 15:28
@dom96 dom96 force-pushed the dominik/refactor-req-gathering branch 2 times, most recently from d220ed6 to f8e3ebd Compare February 14, 2025 14:15
@dom96 dom96 force-pushed the dominik/refactor-req-gathering branch 2 times, most recently from d4c389e to 3895bf9 Compare February 18, 2025 17:25
@dom96
Copy link
Collaborator Author

dom96 commented Feb 18, 2025

For now I've reverted back to using an Array, let's get this merged in and we can use a HashSet later if we agree on merging #3550.

@danlapid
Copy link
Collaborator

For now I've reverted back to using an Array, let's get this merged in and we can use a HashSet later if we agree on merging #3550.

Wasn't following too closely but it seems like you do have agreement on merging that 🤔

Copy link
Contributor

@hoodmane hoodmane left a comment

Choose a reason for hiding this comment

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

Generally looks good to me, but I have one nit I'd like addressed.

@dom96
Copy link
Collaborator Author

dom96 commented Feb 20, 2025

Wasn't following too closely but it seems like you do have agreement on merging that 🤔

Potentially yes, but no approval on the PR as of yet.

@dom96 dom96 force-pushed the dominik/refactor-req-gathering branch 3 times, most recently from 34a1e08 to 1bf6779 Compare February 21, 2025 12:33
@dom96 dom96 force-pushed the dominik/refactor-req-gathering branch from 1bf6779 to 5588d13 Compare February 21, 2025 15:00
@dom96 dom96 merged commit ce57483 into main Feb 21, 2025
17 checks passed
@dom96 dom96 deleted the dominik/refactor-req-gathering branch February 21, 2025 15:49
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.

4 participants