Drop INSTALLED_VERSION

[xavfernandez]

closes #703

This refactor is a first step to ease the transition to a simpler Finder, only returning a list of InstallationCandidate (or alternatively only the best/None) from a req_name and a version specifier (i.e. something like django>=1.5,<1.7).

The logic of satisfied_by/upgrade could/should be performed in an other place.

[xavfernandez]

cc @qwcode since you created #703

[rbtcollins]

I'm not quite sure what this is doing...

The stopping of sorting by location is dubious - I thnk #928 should be closed won'tfix - a fundamental assumptio n is that two distributions of X with version Y are equivalent.

[qwcode]

@xavfernandez I can look at this more tomorrow, but to be clear, I wasn't imagining this much refactor when I wrote #703 3 years ago. At the time, I think I was just thinking that INSTALLED_VERSION didn't need to be based on the inf object since we weren't using the sorting features of inf. So, I think you should explain the refactor more than dubbing it as simply a close for #703.

Also, I don't think you should throw a #928 change into this. that's a behavior change that should be considered separately in another PR IMO.

[xavfernandez]

The stopping of sorting by location is dubious - I thnk #928 should be closed won'tfix - a fundamental assumptio n is that two distributions of X with version Y are equivalent.

Well I don't see a good reason to prefer http://zzz.org/foo-1.0.tar.gz to http://aaa.pypi.org/foo-1.0.tar.gz. The sorting by location seems dubious to me and only serves one purpose: making sure the INSTALLED_VERSION we added ourself comes first.
I don't think we should commit to always respect the find-links order (as stated in #928) but since pip 8 could provide it with less work (i.e. no sorting), why not provide it ?

Edited: but you're right #928 is dubious and was closed. The last commit does not reference it anymore.

[xavfernandez]

@qwcode the refactor aims at extracting the logic of what to do with the installed version (req.satisfied_by) and upgrade outside of the InstallationCandidate sorting.

The behavior before/after the refactor does not change:

satisfied	applicable_versions	upgrade	expected result
y	y	y	return applicable_versions[0] or raise BestInstalled
y	y	n	log, satisfied and info on latest (up-to-date or not), return None
y	n	y	log it is the up-to-date, raise BestInstalled
y	n	n	log it is the up-to-date, return None
n	y	y	log list and best, return applicable_versions[0]
n	y	n	log list and best, return applicable_versions[0]
n	n	y	raise DistributionNotFound
n	n	n	raise DistributionNotFound

It just does not rely anymore on the sort by location.

[xavfernandez]

From my point of view, ideally, the finder should only be provided with a requirement 'django>=1.5,<1.7' (a req_name and a version specifier) and only return a list of InstallationCandidate (or alternatively only the best/None).

The logic of satisfied_by/upgrade should be performed in an other place.

[dstufft]

I agree with the idea that finder should only be provided with a requirement and return a list of candidates (or the best candidate).

[xavfernandez]

I agree with the idea that finder should only be provided with a requirement and return a list of candidates (or the best candidate).

I'd say this PR is a refactoring cleanup to make this next change easier.

[qwcode]

1. Again, let's break this PR up into 2 PRs (the refactor and the sorting change). If a PR depends on another, then explain that in the description. If someone comes back to this 6 months later, it will be much clearer. Also, it's clearer for the current participants. For example, Donald is giving a +1 to your motivation for making the refactor, but it's not clear if he's supporting the sorting behavior change. It's unnecessary confusion.

2. Correct me if I'm wrong, but your point that sorting by location is how we keep INSTALLED_VERSION first is wrong I think. We put it in front using direct insertion and when it is sorted, it's based on a custom key that doesn't use location. If I'm wrong, show me exactly where that happens. btw, the find_links location sorting happens in this line: file_versions.sort(reverse=True), not in applicable_versions = self._sort_versions(applicable_versions)

3. I'm fine with the motivation to simplify the finder to "only return a list [...]" (to be clear, that change isn't in here, but you're wanting that later. that's the kind of remark that would be great in the description for the refactor PR)

4. I agree that the sorting by location is odd, but it's a fact at the moment, so we have to consider the ramifications of people possibly depending on it. It's at least changelog worthy (and again better for the change to be isolated in it's own PR if we're going to reference it in the changelog)

5. If we pull out the location sorting, then I'm thinking that we need to ensure some repeatable order, and have a test that we maintain that.

[dstufft]

Donald is giving a +1 to your motivation for making the refactor, but it's not clear if he's supporting the sorting behavior change

I haven't read the code at all so my +1 is only to the idea of refactoring the finders to only find things and move the selection logic elsewhere.

If we pull out the location sorting, then I'm thinking that we need to ensure some repeatable order, and have a test that we maintain that.

Agreed, two invocations of pip should always return the same thing from a finder.

[xavfernandez]

2. Correct me if I'm wrong, but your point that sorting by location is how we keep INSTALLED_VERSION first is wrong I think. We put it in front using direct insertion and when it is sorted, it's based on a custom key that doesn't use location. If I'm wrong, show me exactly where that happens. btw, the find_links location sorting happens in this line: file_versions.sort(reverse=True), not in applicable_versions = self._sort_versions(applicable_versions)

@qwcode you're right :) I missed that, 1acff7ed10912a2 was removed from the PR.

3. I'm fine with the motivation to simplify the finder to "only return a list [...]" (to be clear, that change isn't in here, but you're wanting that later. that's the kind of remark that would be great in the description for the refactor PR)

Good point, I updated it.

Good thing I waited before merging 😀

[qwcode]

ok, cool, it's just a refactor now. looks ok to me.

[xavfernandez]

Thanks :)