search

unisonweb / base_v1

Unison base libraries, published using V1 codebase format
24 stars 14 forks source link

Add two new generators #24

Closed runarorama closed 4 years ago

runarorama commented 4 years ago

This adds generators for Lists with at least one element, as well as Sets.

Code review

The changes summarized below are available for you to review, using the following command:

pull-request.load https://github.com/unisonweb/base https://github.com/runarorama/base:.addGens

Added definitions:

 test.v1.gen.List.nonEmpty     : '{Gen} [Nat] (+3 metadata)
 test.v1.gen.List.nonEmpty.doc : Doc (+2 metadata)
 test.v1.set                   : '{Gen} a -> '{Gen} Set a (+3 metadata)
 test.v1.set.doc               : Doc (+2 metadata)
pchiusano commented 4 years ago

@runarorama I tried loading this using latest master, got:

unison: user error (😞 I was trying to copy the definition of #9epr0, but I couldn't find it as a type _or_ a term.)

Can you see if that hash is available in your local codebase? Just with view #9epr0.

Also, if you pushed your change using a version of Unison that was before @aryairani recent push / pull stuff then it's possible you could have pushed a definition without all its needed transitive dependencies.

runarorama commented 4 years ago

9epr0 is something really old from my codebase, a deleted term:

  .exampleValue#9epr0o7vi2 : Nat
  .exampleValue#9epr0o7vi2 =
    use Nat +
    2 + 2
runarorama commented 4 years ago

This is pushed using master as of right now (commit #01bbd189).

pchiusano commented 4 years ago

Okay pushed in 8bae9080f13bfa746ddcd8f94ef66eca3e73442d which is Unison namespace hash 7egbi06o86neplebd54g7o15po0kdp8l22v06ug8f9km9lfsgikf5mji

I used gen.nats1 for the name of the List.nonEmpty specialized to Nat.

WDYT about adding a list1 function that takes a '{Gen} a -> '{Gen} [a], analogous to list or set?

I don't really get why that problematic definition 9epr0o7vi2 would be referenced by your namespace though. My only guess is that your definitions weren't atop a clean fork of the current base but had merged in some old namespace you were using for development before the fixes to push. I worked around by just copying the new definitions to a fresh namespace (see transcript below).

https://www.unisonweb.org/docs/codebase-organization#day-to-day-development-creating-and-merging-pull-requests suggests putting new definitions in a totally fresh, empty namespace with parallel naming to where you intend to merge. Starting with a fresh namespace for new stuff also makes it easier to do various bulk operations on just the new definitions being added (like say you want to attach some metadata to everything, or move all the definitions to a subnamepace).

.> cd .runar
.runar> pull-request.load https://github.com/unisonweb/base https://github.com/runarorama/base:.addGens
.runar> fork base merged2
.runar> alias.term merged.test.v1.gen.nats1 merged2.test.v1.gen.nats1
.runar> alias.term merged.test.v1.gen.nats1.doc merged2.test.v1.gen.nats1.doc
.runar> alias.term merged.test.v1.set.doc merged2.test.v1.set.doc
.runar> alias.term merged.test.v1.set merged2.test.v1.set
.runar> diff.namespace base merged2

  Added definitions:

    1. test.v1.gen.nats1     : '{Gen} [Nat] (+3 metadata)
    2. test.v1.gen.nats1.doc : Doc (+2 metadata)
    3. test.v1.set           : '{Gen} a -> '{Gen} Set a (+3 metadata)
    4. test.v1.set.doc       : Doc (+2 metadata)

.runar> cd merged2
.runar.merged2> push git@github.com:unisonweb/base
runarorama commented 4 years ago

I kind of think list1 falls under "totally arbitrary and unnecessary abbreviation", but otherwise I like the idea.