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

Better fallbacks #177

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

benburrill
Copy link

Since get-pipsi.py doesn't create a package_info.json file, pipsi list will currently result in an error when pipsi is installed from master. This PR fixes that bug, as well as generally improving pipsi's package_info fallbacks. See 749c46c for more info.

I didn't bother to make get-pipsi.py actually create the package info file, but it might make sense to do that as well at some point.

Most importantly, this commit adds a fallback for when no
package_info.json file exists, as is the case for the pipsi venv when
installed through get-pipsi.py.  I also moved all the fallback code to
get_package_info.  In doing so, I improved the list_everything fallbacks
which prior to this change gave missing data (like None for version).  I
also believe that putting all the fallback code in one place will make
it easier to maintain.

Additionally, I changed list_everything to actually make use of the name
stored in package_info, which was currently unused.  There are probably
other places that could make use of it too, since if all the code just
goes by the venv name, the name data might as well not even exist.
Call it as a method, also convert from generator to list.
@sciyoshi
Copy link

@benburrill can you see if #161 solves the same issue?

@benburrill
Copy link
Author

benburrill commented Sep 28, 2018

I haven't tested it, but it seems to have a similar goal. One difference I see is that my PR will automatically fill in missing package info even if the file exists, whereas #161 mostly just falls back to None in that case. This means that my PR probably has better backwards compatibility when dealing with packages installed with an old version of pipsi.

A even better solution than either PR might be to get rid of package_info.json altogether. I don't really know why it exists in the first place since the info can be discovered as needed (and already is for the fallbacks).

@reorx
Copy link
Contributor

reorx commented Oct 2, 2018

We share the same goal of fixing the packaging_info.json compatibility and not existing problem. Actually #161 doesn't fall back to None if that file does not exists, in whatever case, a PackageInfo instance will always be created, either read from the json file, or infer from the environment info. Please take a look at this two commit messages :)

@reorx
Copy link
Contributor

reorx commented Oct 2, 2018

A even better solution than either PR might be to get rid of package_info.json altogether.

This was also my first thought after finding out its original purpose (#78).

But then I considered some other functionalities, like I want pipsi to install different versions of same package at the same time, if this is achieved, then directory name would not be simply seen as the package name, at that time package_info.json should be useful for storing the meta data of package info of the directory it lies in.

So instead of removing it I chose to enhance it for future use (hopefully) :)

@benburrill
Copy link
Author

benburrill commented Oct 2, 2018

Actually #161 doesn't fall back to None if that file does not exists

I think you misunderstand. I know that your PackageInfo class falls back appropriately when the file does not exist. My concern (and it could be argued that this isn't such a big deal) is that when the file does exist, create_from_json replaces missing values with None, or in the case of 'scripts', with []. The reason this matters is that when package_info.json was initially created, it only had 'name' and 'version', so these are the only keys we can be confident exist if package_info.json exists. 'scripts' was added in #109. If anyone installed packages using a pipsi between #78 and #109 and then later upgraded pipsi, your PackageInfo class would say that those packages have no scripts, even if they do.

install different versions of same package at the same time

I know this is just an example, but would that even be useful? Since the point of pipsi is to install scripts, the scripts from one version of the package would shadow scripts from the other version.

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.

3 participants