search

mitsuhiko / pipsi

pip script installer
Other
2k stars 133 forks source link

Better fallbacks #177

Open benburrill opened 6 years ago

benburrill commented 6 years ago

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.

sciyoshi commented 6 years ago

@benburrill can you see if https://github.com/mitsuhiko/pipsi/pull/161 solves the same issue?

benburrill commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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.