-
Notifications
You must be signed in to change notification settings - Fork 97
Consider keeping compare module level function #258
Copy link
Copy link
Closed
Labels
EnhancementNot a bug, but increases or improves in value, quality, desirability, or attractivenessNot a bug, but increases or improves in value, quality, desirability, or attractivenessQuestionUnclear or open issue subject for debateUnclear or open issue subject for debateRelease_3.x.yOnly for the major release 3Only for the major release 3
Metadata
Metadata
Assignees
Labels
EnhancementNot a bug, but increases or improves in value, quality, desirability, or attractivenessNot a bug, but increases or improves in value, quality, desirability, or attractivenessQuestionUnclear or open issue subject for debateUnclear or open issue subject for debateRelease_3.x.yOnly for the major release 3Only for the major release 3
Type
Fields
Give feedbackNo fields configured for issues without a type.
I would think that comparing two semver strings is a pretty common use case and this code:
is arguably a lot nicer than
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.compareaccepts theotherargument as a string says a lot about the convenience and desirability of working with strings. But it leaves us in this weird situation whereVersionInfo.comparetakes 2 arguments (self, andother) whereselfmust be aVersionInfoobject butothercan be one of many types. If we were to write out the type signature we would have something like:(Another quirk of this function is that given a tuple as an argument, it will convert to
VersionInfoand 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
VersionInfoobject? 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
VersionInfoAPI. 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 makingsemver.comparelook like this:And I would probably avoid double conversions on lists and tuples.