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

Port install into PackageManager #119

Merged
merged 3 commits into from
Jul 8, 2024
Merged

Conversation

RulerOfCakes
Copy link
Contributor

@RulerOfCakes RulerOfCakes commented Jul 6, 2024

  • ported install into PackageManager
  • moved implementation of install to parent directory(to avoid importing _commands.install.py from package_manager.py)

This concludes implementing the global commands in PackageManager, thus making it viable for use, although it doesn't really serve a separated purpose for end users at the moment. Adding customisability to inner behavior via this abstraction may be a later goal.

Copy link
Member

@ryanking13 ryanking13 left a comment

Choose a reason for hiding this comment

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

Thanks @RulerOfCakes!

"""

def __init__(self) -> None:
self.index_urls = package_index.DEFAULT_INDEX_URLS
self.index_urls = package_index.DEFAULT_INDEX_URLS[:]
Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -6,31 +6,52 @@
from micropip import package_index
from micropip._commands import mock_package
from micropip.freeze import freeze_lockfile
from micropip.install import install
from micropip.list import list_installed_packages
from micropip.package import PackageDict
Copy link
Member

Choose a reason for hiding this comment

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

Let's change import paths to relative paths.

from .package import PackageDict
# same for other imports

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, but I'm curious about the reasoning behind this change? (Personally I don't see a significant advantage over one another)

Copy link
Member

Choose a reason for hiding this comment

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

It distinguishes the local imports from external imports. Also, a minor benefit would be that if we change the package name, it does not need to be updated.

@RulerOfCakes RulerOfCakes marked this pull request as ready for review July 8, 2024 11:14
@ryanking13
Copy link
Member

Thanks!

@ryanking13 ryanking13 merged commit 5d4a869 into pyodide:main Jul 8, 2024
5 checks passed
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