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: minor cleanup and refactor of install funcs #19656

Closed
wants to merge 5 commits into from

Conversation

ttytm
Copy link
Member

@ttytm ttytm commented Oct 25, 2023

I'm about to submit version-installs to allow to fix installations via vpm and dependencies to a tag. E.g.: v install https://github.com/vlang/vsl@v0.1.50 / v install vsl@0.1.50. Also some other fixes.

Some things could be cleaned up and solved more optimally in the current code. They are submitted separately before, as I think it would get a bit messy if they are mixed with other changes.

🤖 Generated by Copilot at 5ba25f3

Refactor and simplify vpm.v to support different vcs for installing modules. Extract a common install_module_from_vcs function and use a switch statement to handle different vcs types.

🤖 Generated by Copilot at 5ba25f3

  • Extract install_module function to avoid duplication and improve readability and maintainability (link, link, link)
  • Simplify and generalize vpm_install function to handle any supported vcs as the source argument (link, link)
  • Simplify vpm_once_filter and vcs_used_in_dir functions by using built-in filter method on arrays (link, link)
  • Rename vcs_key parameter to vcs in vpm_install_from_vcs function for consistency and clarity (link)
  • Remove unnecessary comment line in vpm_install_from_vpm function (link)
  • Simplify return value and use of vcs variable in vcs_used_in_dir function (link, link)

@ttytm ttytm force-pushed the vpm/refactor-cleanup branch 3 times, most recently from fd86dcb to 82fdf51 Compare October 25, 2023 18:08
@ttytm
Copy link
Member Author

ttytm commented Oct 25, 2023

In the end, the last commit breaks with keeping it a "minor cleanup" for this PR. Since the changes attempt to make a good improvement I hope the hassle reviewing it is okay.

@spytheman
Copy link
Member

spytheman commented Oct 26, 2023

Sorry, but no. That hides too many opportunities for breaking stuff, and then when that happens, the only sensible thing would be to revert it, since too many things were changed at once :-| .

I suggest the following plan, done in separate PRs, instead:

  1. Make the reorganization into a cmd/tools/vpm/ subfolder, WITHOUT any other changes,
    just move the vpm.v file there.

  2. Add tests in that cmd/tools/vpm folder. There should be separate tests for installing a module with a github link directly like v install https://github.com/vlang/markdown, as well as installing a module through a vpm identifier, like v install nedpal.args, and also for installing v install pcre (short name, since it is under the vlang org).

  3. Refactor stuff, and change things, keeping the existing tests running and adding more.

@spytheman spytheman closed this Oct 26, 2023
@ttytm
Copy link
Member Author

ttytm commented Oct 26, 2023

Yep let's do so 👍

@ttytm ttytm deleted the vpm/refactor-cleanup branch November 1, 2023 19:47
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