Consolidate packaging functions into framework.package_deps
#1429
Conversation
Queries and tests of installed package versions were spread across a few different modules. Here they are combined into one module (`package_deps.py`) as suggested in qiskit-community#1427.
| If ``package`` is not installed. | ||
| """ | ||
| installed_version = Version(metadata_version(package)) | ||
| return installed_version > Version(f"{version}.dev0") |
There was a problem hiding this comment.
This is nice! It seems like it can't handle some unusual version strings but I don't think we would encounter any of them. Perhaps we could catch any potential errors here just to be safe.
There was a problem hiding this comment.
I see the potential problem, but what should we do after catching an error? Here we are only set up to return a boolean and if we can't parse then we can't say either way. I could add some custom parsing but that leads to the debate you linked to.
There was a problem hiding this comment.
One idea -- PEP440 is pretty old. Anything we are likely to care about should be following it by now. So if we get a version that doesn't parse, we can assume it is old and return False.
There was a problem hiding this comment.
I like that! And emit a warning about the version string.
There was a problem hiding this comment.
Let me know if the new commit looks good.
There was a problem hiding this comment.
The new code looks good. Should the greater than sign be greater than and equal to though? Right now 0.5.0.dev1 is considered to be at least 0.5.0 but 0.5.0.dev0 is not.
There was a problem hiding this comment.
Ah, good, point. I will fix that and add the PR to the merge queue unless you notice something else 🙂
Co-authored-by: Helena Zhang <Helena.Zhang@ibm.com>
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Queries and tests of installed package versions were spread across a few different modules. Here they are combined into one module (
package_deps.py) as suggested in#1427.