Closed runarorama closed 4 years ago
@runarorama cool!!! Can you adjust this to go with the conventions described here on where to put tests, etc. Plays nicer with suffix-based name resolution.
Sorry will be a little tedious to update. :|
I think this now conforms to the conventions.
Removed some controversial aliases
@runarorama okay, reviewed this some more. I realized some neato stuff!
In general, we shouldn't use patches / updates unless the new definition is basically always to be preferred to the old. The old versions of init
and tail
are perfectly cromulent functions that aren't buggy or anything, it's just that we decided we liked those names being used to refer to different definitions. All good. But just because we think those names are better suited for different defs doesn't mean we want anyone using the existing defs to go through a refactoring session. If Alice has written code using the old init
and tail
, do we want to force her to upgrade when she gets the latest base and applies the patch to her code? No not really.
That is, other languages force upgrades on people since any name change is a forced upgrade. For Unison, the choice of doing a name change is separate from the choice of whether the user should have to do any work to upgrade.
To reassign existing names to new definitions without adding to the patch, you would first do a rename.term
(or a delete.term
) on the old definitions (maybe drop1
and drop1End
), then introduce the new definitions with the new type signatures and the names init
and tail
. Nothing is recorded in the patch then, and anyone upgrading their usage of base by applying the latest base patch to their code won't be affected, other than maybe seeing different names for definitions.
Cases where you'd maybe do an actual update vs just deleting or renaming the old one:
What I think is super cool about this is how it means less churn for users of the ecosystem. You aren't forced to deal with needless upgrade cycles just because the maintainers of libraries have evolving ideas about what to call things. :)
Due to some ability inference limitations (this issue for example) functions like Nat.times
get not great inferred types:
Nat.times : Nat ->{𝕖} (a ->{𝕖} a) ->{𝕖} a ->{𝕖} a
That should not need any abilities after the first or second arguments, but the signature as given means you can't even partially apply it and write times 4
inside a pure function. Pretty sure the implementation will typecheck if you gave it the more general signature Nat -> (a ->{e} a) -> a ->{e} a
.
Can you go through all the ability functions and just add ability annotations manually to be exactly what you want and would expect, even when the function is partially applied?
Due to this issue, you get a weird type for an inferred thunk:
Nat.tests.gens.smallNat : ∀ (). () ->{Gen} Nat
This is just because 'a
is literally parsed as _ -> a
, with a variable name of ()
. We should fix that, but in meantime, I'd just add an explicit annotation to these as well ('{Gen} Nat
should do oit) so we avoid weird looking types.
How do I remove init
from the patch?
@pchiusano I believe I've addressed all the issues you raised.
Is this OK to merge?
@runarorama sorry missed original notification, lemme take a look. Thanks for addressing.
I think I still want to hold off on merging more stuff to base until we get to the bottom of the extra namespace files. @aryairani
Okay, we discussed, and going to close and break this one up. Also it's affected by https://github.com/unisonweb/unison/issues/1381 so history should probably get tossed out and relevant definitions copied over.
This also changes the type of
List.init
to be in line withList.head
,List.tail
, etc.The changes summarized below are available for you to review, using the following command:
Updates:
Added definitions: