More refactoring of req_set

[rbtcollins]

No description provided.

[xavfernandez]

Really nice refactor :)
The only thing I dislike is the find_requirement caching on the finder. I would find it cleaner to store/save it in the req_to_install object.

[dstufft]

I think the conditional logic has changed here. Previously these logging statements would only ever happen if req_to_install.editable was False and req.ignore_installed was also False, however now the logging statement will be attempted if req_to_install.editable is False only.

I think the

if req_to_install.satisfied_by:
    skip_reason = self._check_skip_installed(req_to_install, finder)
if req_to_install.satisfied_by:
    logger.info(...)

was changed to

if req_to_install.satisfied_by:
    skip_reason = self._check_skip_installed(req_to_install, finder)
    if req_to_install.satisfied_by:
        logger.info(...)

That would restore the old conditional logic. It would also fix a problem where skip_reason might be unset (If --ignore-installed is being used and the item is already installed).

[dstufft]

Hmm, doing that here would change the logging down below though, since that needs to know if we're supposed to skip install or not to know if it needs to execute, not just execute only when we are ignoring the install.

[dstufft]

Ok, so maybe the real answer here is to change

if req_to_install.satisfied_by:
    skip_reason = self._check_skip_installed(req_to_install, finder)
if req_to_install.satisfied_by:
    logger.info(...)
else:
    if req_to_install.link and req_to_install.link.scheme == "file"):
        path = url_to_path(req_to_install.link.url)
        logger.info(...)
    else:
        logger.info(...)

to

skip_reason = None
if req_to_install.satisfied_by:
    skip_reason = self._check_skip_installed(req_to_install, finder)

if skip_reason is not None and req_to_install.satisfied_by:
    logger.info(...)
else:
    if req_to_install.link and req_to_install.link.scheme == "file"):
        path = url_to_path(req_to_install.link.url)
        logger.info(...)
    else:
        logger.info(...)

I'd also prefer an empty line between the two if statement chains just to separate them more cleanly (as I showed in my example).

[dstufft]

Looking further, I wonder if we really need the skip_reason variable. It appears that the only time this will actually be skipped is if the version is up to date. So we could just assume that's what it is and drop that variable all together? In that case your code as written is fine, just move the up-to-date information into the string and return a boolean from _check_skip_installed() instead. This might work better with the code down below.

I'd still like the newline though.

[dstufft]

Overall I like this cleanup, but I added some comments to it.