-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Support index preference. #264
Conversation
Thanks for the patch for Issue #12, I'm currently triaging bugs, as this is a slightly bigger enhancement I want to get some discussion on it. |
How's the discussion on this patch progressing? It'd be great to see this incorporated into the 1.1 release. =) |
The patch looks good. The only thing I would like to do is trying to extract some methods to make it easier to do unit tests, not only functional behavior tests - I don't know if it is so easy, because PackageFinder is too coupled and |
Sorry for the delay in getting back to this. Can you ensure your change is against current develop branch. |
@@ -48,6 +49,10 @@ def __init__(self, find_links, index_urls, | |||
else: | |||
self.mirror_urls = [] | |||
|
|||
all_origins = self.index_urls + self.mirror_urls + self.find_links | |||
self.origin_preferences = dict([(e[1], e[0]) for e in enumerate(all_origins)]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the fact dicts are unordered this needs to be something else, e.g. a tuple of two-tuples that can be later turned into a dict for easier access.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking at this, Jannis. I need to update this as Paul requested, to refamiliarize myself with my own patch, but I don't believe this is necessary. The origin_preferences dict is only used to get the numeric preference value for the index URL, then that's used to sort the applicable versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, gotcha. thanks!
I've updated my fork and improved the patch. All tests pass. Let me know if it needs anything else. |
Can you look at the failures:
|
Dunno what to do about the Mercurial/Bazaar test failures under it.
OK, I guess the VCS failures were specific to my local environment. |
LGTM :) |
@pnasrat Hey Paul. Any chance we could get this merged? 😄 |
Sure - at wedding this weekend. Will look next week. |
No problem. Hope you had a great time at the wedding. Looking forward to getting this merged. 😄 |
Hi @pnasrat. Just checking in. Any chance we can get this merged? |
Mostly been life stuff (which I've more of currently). I'm happy with the code and tests - you probably need to ensure mergable and I'll merge on green (but may be a week or so on that due to afore-said life) |
Only apply index preference to index, mirror, and find-links sources. Add 1-second delay to test_upgrade tests, to prevent occasional failures when mtime resolution is insufficient. Change test_finder.test_finder_priority_nonegg_over_eggfragments to use file:// URLs instead of HTTP; the non-existent host caused a roughly 20-second delay.
OK, I think I've completely borked this pull request with my mad Github skills. I'm closing this one out and attempting to attach a new one to issue #12. |
This is for #12; should be easier to review and integrate than my old fork on bitbucket.