Skip to content

Conversation

@nmacinnis
Copy link
Contributor

Adds an optional --index-url argument so that hashin can be used with an alternate package index.

Pip supports this behavior with the same argument. Hashin was hardcoded to point at pypi.org, so if you happened to be working with an alternate index, hashin couldn't help you.

Pip also accepts an --extra-index-url argument, but I'm guessing that hashin won't benefit much from supporting the same. I expect the main use case of --index-url to be a package or package version that only exists on the alternate index.

Copy link
Owner

@peterbe peterbe left a comment

Choose a reason for hiding this comment

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

If you accept my suggestion for using a constant then the string "https://pypi.org" only needs to appear exactly 1 single time. Both in the main file and in the unit test(s).

hashin.py Outdated

def get_package_data(package, verbose=False):
url = "https://pypi.org/pypi/%s/json" % package
def get_package_data(package, verbose=False, index_url=None):
Copy link
Owner

Choose a reason for hiding this comment

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

It doesn't make sense to let this be None. It could be ..., index_url='https://pypi.org') but then you'd have to write that string twice. Once here and once in the argparse set-up stuff below.

I think I'd rather this simply be another mandatory positional argument.

hashin.py Outdated
parser.add_argument(
"--index-url",
help="package index url (default https://pypi.org/)",
default="https://pypi.org/",
Copy link
Owner

Choose a reason for hiding this comment

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

How about moving this to the top of the file. Something like this:

DEFAULT_INDEX_URL = os.environ.get('INDEX_URL', 'https://pypi.org')

and then, here, do...:

parser.add_argument(
        "--index-url",
        help="package index url (default {0})".format(DEFAULT_INDEX_URL),
        default=DEFAULT_INDEX_URL,

@nmacinnis
Copy link
Contributor Author

@peterbe thanks for the feedback!

I was already looking at reducing the size of the changeset by defaulting the index url kwarg in a few places. I've incorporated the env var idea. Also made the index url kwarg into a regular positional argument in get_package_data because it is obviously mandatory as you noted.

Copy link
Owner

@peterbe peterbe left a comment

Choose a reason for hiding this comment

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

Love it!

I'll update the README when I make a release.

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