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

Fix version of tools installed by pip #1242

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sara-rn
Copy link
Contributor

@sara-rn sara-rn commented Jan 17, 2025

Closes #1240

Change the version parameter to required and change tools installed with PIP to include the version

@sara-rn sara-rn self-assigned this Jan 17, 2025
@sara-rn sara-rn requested review from Ana06 and removed request for Ana06 January 17, 2025 11:58
Copy link
Member

@Ana06 Ana06 left a comment

Choose a reason for hiding this comment

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

Thanks for adding the versions @sara-rn!! 💐 As noticed by the linter, you have forgotten to update the version in the ipython.vm nuspec. Also, ensure to follow good commit practices such as limiting the subject line to 50 characters and using the body to explain what and why you make the changes. I recommend reading https://cbea.ms/git-commit for more info on best practices to write good commit messages. 😉

@@ -1761,7 +1761,7 @@ function VM-Install-With-Pip {
[string] $toolName, # Example: magika
[Parameter(Mandatory=$true)]
[string] $category,
[Parameter(Mandatory=$false)]
[Parameter(Mandatory=$true)]
Copy link
Member

Choose a reason for hiding this comment

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

the common nuspec also needs update. Please run the linter locally or check the CI status after sending the changes.

Copy link
Member

Choose a reason for hiding this comment

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

This will cause a conflict with #1243 which also updates common. As the other PR has more changes (and is more difficult to rebase), I suggest you wait to do this change after that PR has been merged, to avoid the merge conflicts.

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.

Fix version of tools installed by pip
2 participants