-
Notifications
You must be signed in to change notification settings - Fork 96
Description
I would think that comparing two semver strings is a pretty common use case and this code:
semver.compare("1.2.3", "3.2.1")is arguably a lot nicer than
semver.VersionInfo.parse("1.2.3").compare("3.2.1")plus it is probably already present in a large number of projects that use this library, so removing it entirely should be done only if absolutely necessary (which I do not believe that it is).
The fact that VersionInfo.compare accepts the other argument as a string says a lot about the convenience and desirability of working with strings. But it leaves us in this weird situation where VersionInfo.compare takes 2 arguments (self, and other) where self must be a VersionInfo object but other can be one of many types. If we were to write out the type signature we would have something like:
VersionInfo.compare(self: VersionInfo, other: Union[str, dict, list, tuple, VersionInfo])(Another quirk of this function is that given a tuple as an argument, it will convert to VersionInfo and then immediately back to a tuple again.)
I think as far as API design goes, it doesn't make a lot of sense for the main comparison function to have this type signature. Why is only one argument required to be a parsed VersionInfo object? Why are we doing automatic conversion on one argument but not the other? What if I already have both arguments in tuple form and don't want to perform two useless conversions?
I would suggest having the module level API be a sort of convenience wrapper around the VersionInfo API. I think it is likely that the vast majority of semver data starts out in string form, so there should be a quick and clean API that one can call on this data. I would suggest making semver.compare look like this:
semver.compare(left: Union[str, dict, list, tuple, VersionInfo], right: Union[str, dict, list, tuple, VersionInfo])And I would probably avoid double conversions on lists and tuples.