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

vpm: add parse_query #19814

Merged
merged 1 commit into from
Nov 9, 2023
Merged

vpm: add parse_query #19814

merged 1 commit into from
Nov 9, 2023

Conversation

ttytm
Copy link
Member

@ttytm ttytm commented Nov 9, 2023

Based on the end result of #19813 we can make it a 3 or 4 part change which can be better monitored.

This PRs main change:

  • Add a parse_query step - to evaluate the requested modules (and versions to use in a followup PR) at the beginning and use []Module arrays that store all data that is needed instead of working with []string arrays for the whole process.

fn get_mod_date_info(mut pp pool.PoolProcessor, idx int, wid int) &ModDateInfo {
mut result := &ModDateInfo{
fn parse_query(query []string) ([]Module, []Module) {
mut vpm_modules, mut extended_modules := []Module{}, []Module{}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
mut vpm_modules, mut extended_modules := []Module{}, []Module{}
mut vpm_modules, mut external_modules := []Module{}, []Module{}

Copy link
Member

Choose a reason for hiding this comment

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

or https_modules

Copy link
Member Author

@ttytm ttytm Nov 9, 2023

Choose a reason for hiding this comment

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

Good catch, ty. https would be even shorter. But the install method can be extended to also support other instal methods. So external would cover this as well.

Copy link
Member

Choose a reason for hiding this comment

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

it can be changed in a future PR to avoid triggering the CI just for that

@@ -28,25 +29,22 @@ fn vpm_install(requested_modules []string) {
exit(2)
}
} else {
requested_modules.clone()
Copy link
Member

@spytheman spytheman Nov 9, 2023

Choose a reason for hiding this comment

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

I think that this return input_param.clone() here, was deliberate, to avoid a problem with V itself, when vpm_install was called with []string on the stack, and then the result was passed towards a higher level caller. I am not sure if that is fixed already, it may be 🤔 , just something to keep in mind.

Copy link
Member Author

@ttytm ttytm Nov 9, 2023

Choose a reason for hiding this comment

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

Afaik the clone was needed as we did a direct assignment. Now we pass it to the function that returns two new arrays.

@spytheman spytheman merged commit 10160c0 into vlang:master Nov 9, 2023
43 checks passed
@ttytm ttytm deleted the vpm/parse-query branch November 13, 2023 14:45
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.

2 participants