-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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
ModuleSourceRemote String now considers query string (#31250) #31636
Conversation
Thanks for this submission. Although I cannot commit to having this PR reviewed at this time, we acknowledge your contribution and appreciate it! |
Hi @fatahfattah, if you could please write one or more tests for this change (even simple tests will be fine), we can get this reviewed. Thanks again for your submission! |
5485b30
to
d98f4a6
Compare
45893ad
to
cd85b5c
Compare
cd85b5c
to
f4b6d27
Compare
Hi @crw I added a few tests, please let me know if they are okay or if you require some changes. I noticed there was no separate testing suite for parsing the ModuleSourceRemote so I added that as well. |
The team member who is going to review this is on vacation, but we'll make sure to review this as soon as he is back. Thanks! |
Thanks for working on this @fatahfattah! This implementation looks good to me, and so I'm going to merge it now. |
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
This fixes an issue in the String() for ModuleSourceRemote in which it does not consider query strings (if present). Before this fix, any subdirectory would simply be appended to the ModuleSourceRemote.Package instead of inserting it between the root package and any query parameters.
Related: #31250