#264 ❤️ #257 #267

armanbilge · 2021-09-03T21:48:56Z

Marries #264 and #257. Only remaining thing to consider is adding map and contramap to LookupKey and InsertKey.

tpolecat · 2021-09-03T21:55:51Z

core/src/main/scala/org/typelevel/vault/InvariantMapping.scala

-      val in = g andThen outer.in
-      val out = outer.out andThen f
+      val in = AndThen(g) andThen outer.in
+      val out = AndThen(outer.out) andThen f


Neato, I didn't know about that one.

armanbilge · 2021-09-03T22:11:30Z

core/src/main/scala/org/typelevel/vault/Key.scala

+  private[vault] type I
+  private[vault] def in: A => I
+
+  def contramap[B](f: B => A): InsertKey[B] =


Note that contramap and map will now be exposed on Key itself. It's safe, because you'll end up with an InsertKey/LookupKey after doing this, but it could be confusing.

armanbilge · 2021-09-03T22:16:20Z

Cool, that wasn't bad at all. I think this is good to go.

tpolecat · 2021-09-08T14:42:41Z

Are we waiting on typelevel/cats#3987 here? If not are there any other blockers?

armanbilge · 2021-09-08T14:44:23Z

We're not. Actually, I didn't (want to) use any coyoneda here since that would mean adding a new dependency to cats-free, unless that's okay? So, no blockers.

rossabaker

I think this is good. Some of the overloads are making me fidgety, but I can be convinced.

rossabaker · 2021-09-18T03:01:38Z

.gitignore

@@ -11,3 +11,6 @@ tags
 .vscode
 .bsp
 .jekyll-cache/
+
+# metals
+metals.sbt


💡 I can't remember who I've told, but ~/.config/git/ignore takes care of this on all your repos.

rossabaker · 2021-09-18T03:11:49Z

core/src/main/scala/org/typelevel/vault/Locker.scala

+  def lock[A](k: InsertKey[A], a: A): Locker = new Locker(k.unique, k.in(a))
+
+  /**
+   * Put a single value into a Locker
+   */
+  def lock[A](k: Key[A], a: A): Locker = lock(k: InsertKey[A], a)


Overloads make me queasy and overloads where one is a subtype of another make me queasier still. I guess this is necessary for bincompat? Is there a different name we could give the InsertKey version?

Or could we have just made this private[vault]?

We have the same basic issue with unlock.

This is needed for bincompat, and no private[vault] does not work because it disappears the static forwarders. I ran into this same issue in typelevel/fs2#2466 (comment).

OTOH, there is precedence for not caring about static forwarders in http4s/http4s#5071 (comment).

I'm not sure that was a good precedent. And it seems we're stuck with the overload on delete anyway. I find it a little confusing, but I don't see a good way out.

Maybe an unpopular opinion, one way out would be to deprecate the vault companion object based methods (I always found the duplication confusing, does anyone actually use them like this?) in favor of just calling these methods directly on the vault instance, and then just not add these new methods to the companion object.

I didn't realize they existed on the companions. I think that's a good idea. The only weird one then is the overloaded Lock.lock, right? And perhaps the new one could be called apply?

Cool, let me give that a try 👍

rossabaker · 2021-09-18T03:14:02Z

core/src/main/scala/org/typelevel/vault/Vault.scala

+  /**
+   * Delete a key from the vault
+   */
+  def delete[A](k: InsertKey[A]): Vault = Vault.delete(k, this)
+
+  /**
+   * Delete a key from the vault
+   */
+  def delete[A](k: LookupKey[A]): Vault = Vault.delete(k, this)


Uh, what happens if we call this with Key? Does it pick one from the linearization of Key? It doesn't really matter, I guess?

If we call this with Key it uses the existing overload for Key and ignores both of these.

Oh, I overlooked that there's still a delete(Key[A]).

armanbilge · 2021-09-18T03:41:55Z

@rossabaker the spec on overload resolution is here: https://www.scala-lang.org/files/archive/spec/2.13/06-expressions.html#overloading-resolution

tl;dr: overloaded methods are resolved to the most specific available for the given type. This is why if there are overloads for both InsertKey and LookupKey we must also provide an overload for Key to resolve the ambiguity.

Arman had a better idea

armanbilge · 2021-09-18T22:15:24Z

@rossabaker I just pushed a commit that:

Bumps base version to 3.1 (not strictly necessary since everything is source-compatible, but we are making a lot of changes?).
Deprecates the companion object methods in favour of equivalent instance methods.
Avoids the delete overloads by introducing one more abstraction DeleteKey that is the parent of all keys.

rossabaker

👍 if green

armanbilge · 2021-09-18T22:30:25Z

Good to go.

rossabaker · 2021-09-18T22:42:13Z

I'll leave Chris and Rob a short window to object, but I'd like to release it this weekend and unblock Rob's PR for an http4s release train on Monday.

rossabaker · 2021-09-18T23:02:14Z

Daniel, would love your thoughts here, too, if you've got time.

I think it's good, but this is all a slight move away from Tried And True Direct Haskell Port, so I'm being extra careful with it.

rossabaker · 2021-09-19T21:34:22Z

I'm going to release this. We have a long-blocked and important http4s PR on this.

danicheg

Sorry for being late (vacation in full swing). I think that's great work! And personally see no evil in moving away from direct Haskell port 🙈

rossabaker · 2021-09-20T12:45:48Z

Enjoy your break!

armanbilge and others added 8 commits August 23, 2021 23:37

Introduce InsertKey / LookupKey abstraction

6ab0d7c

Add one more override

b4ca4d3

make Key an invariant functor

889f01d

bincompat

34c6ddb

foratting

a41339a

Merge branch 'topic/variance' into tpolecat/invariant

a558d10

Move vault suite to correct test dir

78b3f95

Use AndThen for stack-safety

ac56ebf

tpolecat reviewed Sep 3, 2021

View reviewed changes

armanbilge added 2 commits September 3, 2021 22:07

Add Functor/Contravariant for Lookup/Insert

73d7be1

Privatize InvariantMapping

2eeace6

armanbilge commented Sep 3, 2021

View reviewed changes

tpolecat mentioned this pull request Sep 13, 2021

Pass WebSocketBuilder to HttpApp in builders http4s/http4s#5152

Merged

1 task

rossabaker reviewed Sep 18, 2021

View reviewed changes

rossabaker previously approved these changes Sep 18, 2021

View reviewed changes

Deprecate companion object methods

c3d7358

Merge branch 'main' into tpolecat/invariant

bf872ec

rossabaker approved these changes Sep 18, 2021

View reviewed changes

Fix deprecated uses in docs

6c2d47e

rossabaker requested a review from ChristopherDavenport September 18, 2021 22:41

rossabaker requested a review from danicheg September 18, 2021 22:59

This was referenced Sep 19, 2021

make Key an invariant functor #264

Closed

Introduce InsertKey[-A] / LookupKey[+A] abstraction to allow for variance #257

Closed

rossabaker merged commit 8fcd3cd into typelevel:main Sep 19, 2021

danicheg reviewed Sep 20, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#264 ❤️ #257 #267

#264 ❤️ #257 #267

armanbilge commented Sep 3, 2021 •

edited

Loading

tpolecat Sep 3, 2021

armanbilge Sep 3, 2021 •

edited

Loading

armanbilge commented Sep 3, 2021

tpolecat commented Sep 8, 2021

armanbilge commented Sep 8, 2021 •

edited

Loading

rossabaker left a comment

rossabaker Sep 18, 2021

rossabaker Sep 18, 2021

rossabaker Sep 18, 2021

rossabaker Sep 18, 2021

armanbilge Sep 18, 2021 •

edited

Loading

rossabaker Sep 18, 2021

armanbilge Sep 18, 2021

rossabaker Sep 18, 2021

armanbilge Sep 18, 2021

rossabaker Sep 18, 2021

armanbilge Sep 18, 2021

rossabaker Sep 18, 2021

armanbilge commented Sep 18, 2021

armanbilge commented Sep 18, 2021 •

edited

Loading

rossabaker left a comment

armanbilge commented Sep 18, 2021

rossabaker commented Sep 18, 2021

rossabaker commented Sep 18, 2021

rossabaker commented Sep 19, 2021

danicheg left a comment

rossabaker commented Sep 20, 2021

#264 ❤️ #257 #267

#264 ❤️ #257 #267

Conversation

armanbilge commented Sep 3, 2021 • edited Loading

Choose a reason for hiding this comment

armanbilge Sep 3, 2021 • edited Loading

Choose a reason for hiding this comment

armanbilge commented Sep 3, 2021

tpolecat commented Sep 8, 2021

armanbilge commented Sep 8, 2021 • edited Loading

rossabaker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armanbilge Sep 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armanbilge commented Sep 18, 2021

armanbilge commented Sep 18, 2021 • edited Loading

rossabaker left a comment

Choose a reason for hiding this comment

armanbilge commented Sep 18, 2021

rossabaker commented Sep 18, 2021

rossabaker commented Sep 18, 2021

rossabaker commented Sep 19, 2021

danicheg left a comment

Choose a reason for hiding this comment

rossabaker commented Sep 20, 2021

armanbilge commented Sep 3, 2021 •

edited

Loading

armanbilge Sep 3, 2021 •

edited

Loading

armanbilge commented Sep 8, 2021 •

edited

Loading

armanbilge Sep 18, 2021 •

edited

Loading

armanbilge commented Sep 18, 2021 •

edited

Loading