purescript / registry-dev

Development work related to the PureScript Registry
https://github.com/purescript/registry
97 stars 80 forks source link

Implement 'Version' and 'Range' types from the spec #541

Closed thomashoneyman closed 1 year ago

thomashoneyman commented 1 year ago

This PR implements the Version and Range types described in the spec (#533). The change is almost entirely mechanical; move over the data types, parser, and printer, and implement a JSON codec. However, I've made some decisions that I think notably improve how we use these types.

First, Version and Range are implemented in their own modules instead of sharing a module. This means that we no longer have to disambiguate version and range operations within the namespace, such as Version.parseVersion and Version.parseRange. It also means that range-specific operations get their own namespace (no more Version.union or Version.intersect, which doesn't make any sense since those are range operations).

Second, there is no longer a ParseMode usable with these types; instead, the registry only uses strict versions and ranges, and the application offers its own LenientVersion and LenientRange types. This is an improvement for a lot of reasons!

  1. We never allow lenient versions in the core registry (things like a Manifest) -- but right now it's up to developer discipline to not accidentally use Version.parseVersion Version.Lenient somewhere only strict versions should be allowed. A dedicated LenientVersion means you definitely expect to be dealing with non-strict versions, which is a big heads-up that this is a place to be careful in the code.
  2. We only use lenient versions and ranges when dealing with legacy formats, which is strictly a registry application concern; contorting around these types literally everywhere else in the application feels like a bad code smell.
  3. We already use these types in error. A function like Range.union makes no sense in the lenient context; you no longer have one "input" you parsed from. And there are places where we use Version.rawVersion to indicate we want to print a raw version, but it could only ever have been possible to have a strict version (ie. Version.printVersion == Version.rawVersion); this is confusing, because either we did want a raw version, in which case this could be a bug, or we didn't, in which case rawVersion should not have been used.

It's much nicer to just separate these concerns altogether, and so I've done just that. But I've kept all the same tests and ensured they all continue to pass.

thomashoneyman commented 1 year ago

As far as testing goes, beyond the unit tests I've also run the registry importer and verified that the import statistics are unchanged. This code change does not appear to affect the behavior of the registry.

thomashoneyman commented 1 year ago

💪 I'll let this sit for a day or two so that @f-f can take a look since it affects his Spago stuff.

f-f commented 1 year ago

Thanks @thomashoneyman, great work! 👏