add initial text #1

acutmore · 2023-06-26T16:52:51Z

brain dump ( duplicate PR to the bloomberg one)

nicolo-ribaudo

I only read the readme so far, but it looks good overall!

nicolo-ribaudo · 2023-06-27T13:23:02Z

README.md

+key1 === key2; // separate objects
+CompositeKey.equal(key1, key2); // true
+CompositeKey.equal(key1, key3); // false
+Reflect.ownKeys(key1); // [] - opaque empty object from the outside


Is it just a {}, or is it an instance of CompositeKey? Is it frozen or mutable?

Good question. I've updated the README now. They are instances of CompositeKey and not frozen. Also I think it was unclear before, that each call returns a fresh instance. So they are more 'normal' in that sense, like other types in the language. But they are special that CompositeKey.equal checks the internal-slot of the argument, so only real instances work.

README.md

mhofman · 2023-07-14T22:42:30Z

README.md

+r1.x; // 0
+r1 === r2; // false
+
+let s = new Set([r1], { keyBy: Symbol.keyBy });


So keyBy is allowed to not be a function? Is this a shorthand for (x) => x[Symbol.keyBy]()?

Yes and yes, though it could have additional semantic meaning such as throwing a TypeError if the value does not contain the string/symbol. The main motivation for this is not changing the default keying semantics of a plain new Set() while also making it convenient to switch to a Map / Set that does look up the protocol.

However this could also be achieved by the keyBy option only accepting a function keeping the core constructor API cleaner and adding a static helper method maybe something like: new Set.keyed(), new KeyedSet(), or new CompositeKey.Set()

mhofman · 2023-07-14T22:50:42Z

README.md

+
+### Map and Set config (phase 1)
+
+Allow `Map` and `Set` instances to be customized with a lookup function that will produce the value that represents the keys values for that collection. In this mode the representative values are considered equal if they are `SameValueZero` equal, or if they are both `CompositeKey`s and equal according to `CompositeKey.equal`.


Should there be a way to opt-out of the CompositeKey special equality semantics in these collections? Without it, how can a program associate data to specific instances of a CompositeKey (besides the return override + private fields stamping hack).

One solution may be to use the CompositeKey equality semantics only when keyBy option is used?

Yeah exactly. The CompositeKey is only treated special when it is returned by the keyBy function.

While I don't disagree with this choice, I'll point out that maybe the answer should be "don't do that"? If CompositeKey just had 100% value semantics when used as a map key then that's one less point of complexity, and I'm struggling to come up with a reasonable use case where someone should want to distinguish. Also, SameValueZero already has differences from any other equality predicate (=== and Object.is, specifically) so the fact that maps are automatically collapsing two things that aren't strictly equal into a single key isn't inconsistent with existing behavior for ±0 or NaN. That said, I think this would actually make it quite a bit harder to polyfill as well. But I wanted to get both sides of the argument on paper.

Yeah I think this could go either way, but really I'm not convinced application developers should be using so many CompositeKeys directly.

mhofman · 2023-07-14T22:56:24Z

README.md

+    - The downside to this is that when comparing if two values are equal by comparing their `CompositeKeys`, both values need to produce a full `CompositeKey` rather than doing this gradually and exiting early as soon as one part does not match. A separate API for this use case could avoid this issue.
+- What about `WeakMap` and `WeakSet`?
+    - More investigation required.
+    - a `keyBy` function for these makes less sense because if the function returns a new value then the only reference to that value will be held weakly and therefore eligible for collection.


I would disagree. For example a keyBy implementation that returns a CompositeKey would have its special equality semantics apply.

mhofman · 2023-07-14T22:57:40Z

README.md

+    - A `new CompositeKey(0, 0)` does not hold any lifetime information, a matching `CompositeKey` can always be created if the _inputs_ are available.
+    - Allowing only `CompositeKeys` that were created from at least one value that itself is allowed as a `WeakMap` key could be an option.


Yes, only composite keys composed of at least one object eligible as WeakMap key would itself be allowed as WeakMap key.

README.md

- CompositeKey construct no longer varargs - `keyBy` Map/Set config only accepts function - Add Map.usingKeys helper instead - Make `Symbol.keyBy` a method rather than a getter

README.md

mhofman · 2023-07-21T17:31:25Z

README.md

+let key1 = new CompositeKey([0, 0]);
+let key2 = new CompositeKey([0, 0]);
+let key3 = new CompositeKey([0, 1]);


Why the switch to requiring an array wrapping the values? I actually find this more confusing, making people think arrays are somehow special.

It was commented that varargs makes it hard/impossible to expand in the future. e.g can't add an options bag.
The first arg could be any iterable similar to Map/Set.

I have also added a CompositeKey.of(v1, v2, ...vs) to keep the varargs style. The thinking here is that this is like Temporal where the constructors are very raw exposing the lowest-level of functionality, and then there are static factories that have nicer APIs.

I suppose there is precedent with the Set and Map constructors.

I like the varargs style better; I don't think we need this to take an array/iterable.

I guess while new Map, Promise.all and friends take iterables; Array is Array(1, 2, 3) (ignoring the footgun overload for 1 argument) and CompositeKey is somewhat like an Array and I can't think of future arguments we may want to pass to the constructor. Varargs does feel cleaner which is why I initially went with it.

mhofman · 2023-07-21T17:56:51Z

README.md

+}
+```
+
+### Records and Tuples (follow on?)


One thing not mentioned in this readme is the difference with the current R&T proposal. As primitive values they round tripped through the ShadowRealm callable boundary, which had a lot of interesting properties.

This may be good to note, but if implementations do not want to add more primitives, then I think this is a good path through.

mhofman · 2023-07-21T17:57:09Z

README.md

+    - ECMA262 aims to be as deterministic as possible (`Date.now()` and `Math.random()` being examples of the few exceptions) and backwards-compatible with previous versions; this would mean that built-in hash functions would most likely need to be fully specified and limited ability to evolve the hashing algorithm. Exposing these low level details _may_ also pose a security risk.
+- What about membranes?
+    - More investigation required.
+    - Out of the box `CompositeKey` won't work across membranes because their uniqueness is encoded within an internal slot. Membranes would need to add explicit support for re-constructing CompositeKeys when used across a membrane.


Yeah existing membranes would need to be updated to become fully transparent. I think we agreed that any new global API is fair game for introducing such a change, as long as there is a path to making the membrane transparent again.

Right, so, in particular, if you pass the constructor over the membrane barrier, it will then wrap its parameters properly and all will be well, except that comparisons in maps may not work. An upgrade to the membrane system would be needed to recover the intended equality semantics, but not provide any sort of "security" issue, right?

So full transparent membranes don't have any intrinsic security properties, the question is whether the membrane is transparent or not.

The problem is when one side expects to reach behavior of an object that isn't available on the object's "surface". In this case, it's whether a program using a Map/Set with keyBy on one side would expect to be able to use a CompositeKey instance from another side, or if the CompositeKey global from one side (as a constructor or its static methods) would expect to be able to use the instances from the other side.

I think it's likely fine to say right now that CompositeKey instances behave like plain objects through a membrane unaware of CompositeKey, and so we simply don't have the special equality semantics.

If CompositeKeys had Symbol.keyBy on them and this was looked up by Map's keyBy that could be where the membrane can add support for object's keys working across membranes. (objects from blue-realm work in the keyBy map of red's realm). The red-proxy-trap on Symbol.keyBy would get blue's compositeKey and use a Map to get a consistent red-object that represents that key, then put that object in a Red CompositeKey and return that.

So yeah, out-of-the-box CK across membranes would only have regular object identity when used as keys, but support could be added.

shicks · 2023-07-24T21:54:28Z

README.md

+        this.value = value; this.leafs = leafs;
+    }
+
+    getKey() {


Maybe this isn't a realistic example...? Having getKey() be O(n) seems less than ideal.

It doesn't map very well, but our "interned value type" alternative to R&T does everything shallowly, which seems like a better approach. But maybe there's no possible analogue with this paradigm.

I think this example is slightly confusing more because of how early it is in the README, and people also might wonder what the relationship between getKey and Symbol.keyBy is. Maybe you could have a much simpler example here, showing that CompositeKey.equals(new CompositeKey(new CompositeKey(1, 2), 3), new CompositeKey(new CompositeKey(1, 2), 3)) === true. And save the classes for when you're actually overloading [Symbol.keyBy] (and then for something fixed-size and not recursive).

shicks · 2023-07-24T22:05:40Z

README.md

+
+### Map and Set config (phase 1)
+
+Allow `Map` and `Set` instances to be customized with a lookup function that will produce the value that represents the keys values for that collection. In this mode the representative values are considered equal if they are `SameValueZero` equal, or if they are both `CompositeKey`s and equal according to `CompositeKey.equal`.


While I don't disagree with this choice, I'll point out that maybe the answer should be "don't do that"? If CompositeKey just had 100% value semantics when used as a map key then that's one less point of complexity, and I'm struggling to come up with a reasonable use case where someone should want to distinguish. Also, SameValueZero already has differences from any other equality predicate (=== and Object.is, specifically) so the fact that maps are automatically collapsing two things that aren't strictly equal into a single key isn't inconsistent with existing behavior for ±0 or NaN. That said, I think this would actually make it quite a bit harder to polyfill as well. But I wanted to get both sides of the argument on paper.

shicks · 2023-07-24T22:16:07Z

README.md

+        return v;
+    }
+}
+new CompositeKey([lookupKey(position1), lookupKey(position2)]);


This seems a little surprising to me, actually. Given the analogue to Array.of, I wouldn't expect any transformation to happen on the elements. I see the value here, but I wonder if a different name wouldn't be prudent to avoid the assumption.

shicks · 2023-07-24T22:23:14Z

README.md

+    - This allows any hashing and equality operations to be done lazily on API access. Compared to `CompositeKey(0) === CompositeKey(0)`, which would require either an eager global intern-table or for `===` to be have a special overload for these objects.
+- Why are the `Map` and `Set` changes opt-in, and do not work with existing default constructors `new Map()` and `new Set()`?
+    - Adding `Symbol.keyBy` to an object could invalidate existing code that assumes `Map` and `Set` will use object identity.
+    - The opt-in mode can be strict, and throw an Error if a value does not implement `Symbol.keyBy` rather than silently falling back to object identity.


If we want to be able to add Symbol.keyBy to existing types without breaking the web then this would be the only way to have any chance at that.

On one hand yes, this is the safest strategy. On the other hand, it might be OK if people don't depend on it too much (sometimes JSON serializability is added later, and that kinda has the same issue). Anyway I prefer this strategy.

I'm still wondering whether we should require that the keyBy option of collection should be a CompositeKey.

Perhaps CompositeKey or primitive? Seems OK for something to want to keyBy a number or string without needed to put that value in a CK.

Ah yes, I meant that. keyBy option of collections would require a primitive or CompositeKey, and similarly the R&T constructors would require that a nested object has a Symbol.keyBy that returns a primitive or CompositeKey. That way it'd be consistent for any Map.usingKeys()

shicks · 2023-07-24T22:29:10Z

README.md

+
+Immutable values types such as those in Temporal could implement `Symbol.keyBy`, without requiring users to work out the best way to represent these types using a `CompositeKey`.
+
+## Q+A


I'd like to see some discussion of mutability here. In particular, I'm convicted that keying a mutable object by any mutable property is a Bad Idea. This was impossible with the old R&T, but nothing here even mentions it as a concern. I don't see a good way to outright remove this footgun, but I wonder how the system will react if (when) it happens. Suppose an object is added to a set under one key and later keyBy returns something different - what happens if the first key is added again?

I think we're talking about "stability of the keyBy function" and not mutability itself. R&T got around this by not exposing a user-defined function. But Bradley correctly determined that this functionality is needed, despite this risk. I think we should just live with this, define exactly what the algorithms do (to imply how they handle inconsistencies--probably by ignoring them) and key things by the returned keyBy.

As we discussed we have 2 options with different trade-offs:

We allow keyBy to return different values for the same object used a collection key. This will result in the possibly of duplicate keys when iterating and potentially the inability to delete some entries.

We disallow keyBy to return a different value for keys already in used in the collection. This effectively requires the implementation to keep an internal map from the key to the previous keyBy result.

Personally I don't see the issue as mutability, but mutation - iow, codebases and APIs that simply don't mutate work just fine without immutability. iow, using objects that are treated as immutable is the same as using actually immutable ones in practice, most of the time.

Also I'd like to support the use case of mutable exits in a record / tuple without symbol indirection like required for a primitive R&T proposal. This can be accomplished in the current state of this proposal by an object implementing the keyBy protocol, which returns CompositeKey([this]). As long as keyBy is required to return a primitive value or CompositeKey value, I think it would express sufficient opt-in to structural equality semantics.

shicks · 2023-07-24T22:31:10Z

README.md

+- What about membranes?
+    - More investigation required.
+    - Out of the box `CompositeKey` won't work across membranes because their uniqueness is encoded within an internal slot. Membranes would need to add explicit support for re-constructing CompositeKeys when used across a membrane.
+- What about `WeakMap` and `WeakSet`?


dumb question, but I suppose it's worth making explicit - is there any reason to allow WeakMaps to have keyBy? My gut says that defeats the purpose, but without working through the detailed implications, I don't have a solid explanation for why.

Probably something like, to avoid the need to build a trie for this purpose. Rick Button made a trie like that when implementing the R&T polyfill. I'm not sure how common a need this really is.

We have one of those trie as well. Actually we have a few, none which would be needed if we had composite keys.
A common use case is memoization of a function based on multiple arguments.

littledan · 2023-07-25T19:24:46Z

README.md

+
+### Map and Set config (phase 1)
+
+Allow `Map` and `Set` instances to be customized with a lookup function that will produce the value that represents the keys values for that collection. In this mode the representative values are considered equal if they are `SameValueZero` equal, or if they are both `CompositeKey`s and equal according to `CompositeKey.equal`.


Yeah I think this could go either way, but really I'm not convinced application developers should be using so many CompositeKeys directly.

littledan · 2023-07-25T19:27:09Z

README.md

+
+Immutable values types such as those in Temporal could implement `Symbol.keyBy`, without requiring users to work out the best way to represent these types using a `CompositeKey`.
+
+## Q+A


I think we're talking about "stability of the keyBy function" and not mutability itself. R&T got around this by not exposing a user-defined function. But Bradley correctly determined that this functionality is needed, despite this risk. I think we should just live with this, define exactly what the algorithms do (to imply how they handle inconsistencies--probably by ignoring them) and key things by the returned keyBy.

littledan · 2023-07-25T19:28:34Z

README.md

+    - This allows any hashing and equality operations to be done lazily on API access. Compared to `CompositeKey(0) === CompositeKey(0)`, which would require either an eager global intern-table or for `===` to be have a special overload for these objects.
+- Why are the `Map` and `Set` changes opt-in, and do not work with existing default constructors `new Map()` and `new Set()`?
+    - Adding `Symbol.keyBy` to an object could invalidate existing code that assumes `Map` and `Set` will use object identity.
+    - The opt-in mode can be strict, and throw an Error if a value does not implement `Symbol.keyBy` rather than silently falling back to object identity.


On one hand yes, this is the safest strategy. On the other hand, it might be OK if people don't depend on it too much (sometimes JSON serializability is added later, and that kinda has the same issue). Anyway I prefer this strategy.

littledan · 2023-07-25T19:30:00Z

README.md

+    - ECMA262 aims to be as deterministic as possible (`Date.now()` and `Math.random()` being examples of the few exceptions) and backwards-compatible with previous versions; this would mean that built-in hash functions would most likely need to be fully specified and limited ability to evolve the hashing algorithm. Exposing these low level details _may_ also pose a security risk.
+- What about membranes?
+    - More investigation required.
+    - Out of the box `CompositeKey` won't work across membranes because their uniqueness is encoded within an internal slot. Membranes would need to add explicit support for re-constructing CompositeKeys when used across a membrane.


Right, so, in particular, if you pass the constructor over the membrane barrier, it will then wrap its parameters properly and all will be well, except that comparisons in maps may not work. An upgrade to the membrane system would be needed to recover the intended equality semantics, but not provide any sort of "security" issue, right?

littledan · 2023-07-25T19:30:43Z

README.md

+- What about membranes?
+    - More investigation required.
+    - Out of the box `CompositeKey` won't work across membranes because their uniqueness is encoded within an internal slot. Membranes would need to add explicit support for re-constructing CompositeKeys when used across a membrane.
+- What about `WeakMap` and `WeakSet`?


Probably something like, to avoid the need to build a trie for this purpose. Rick Button made a trie like that when implementing the R&T polyfill. I'm not sure how common a need this really is.

README.md

mhofman

An advantage of introducing all elements at once is that we can have stronger requirements on the values allowed for each feature, in particular require that [Symbol.keyBy]() returns a primitive or CompositeKey, that R&T similarly require every value to be a primitive or object with Symbol.keyBy, and possibly even that the keyBy option of Collections must return a primitive or CompositeKey.

As I mentioned last time, the program can still express all keying equivalence they wish were these restriction not present by simply wrapping a regular object they would have used directly, into a CompositeKey containing only that object.

mhofman · 2023-08-16T23:26:36Z

README.md

+- How does this compare to the original Record&Tuple proposal?
+    - Here R&T are just plain objects and arrays, not primitives
+    - They do not enforce deeply immutable structures
+    - They can contain any value, including functions


I still think we should make the R&T only accept primitives or objects that have a Symbol.keyBy to require explicitly opting to an exit of an immutable structure.

We could allow R&T to include any value at construction time. It's unlikely that a function would implement Symbol.keyBy but might be common to want to reference a function directly.

Instead the error is delayed until the R/T's Symbol.keyBy is invoked, e.g. when it's put into a keyed Map/Set, and that would then throw a TypeError. This is similar to Python where Tuples can contain values than don't implement hashable, but when trying to get the hash of that tuple it then errors.

This allows users to still easily use R/T if they are only interested in the getting the shallow immutability Object.freeze sugar, and not making use of keyBy.

What do you think?

Well I suppose that highlights another problem: the mutable value in these records/tuples may drop their @@keyBy at anytime, and even if that property doesn't change or is captured at creation, it could change its return value too.

I think we basically need to decide when the [Symbol.keyBy]() call is performed, whether at construction or lazily when the record's [Symbol.keyBy]() is itself called, in which case whether the values should be cached or recomputed every time.

Co-authored-by: Jordan Harband <ljharb@gmail.com>

acutmore · 2023-09-02T14:49:22Z

README.md

+CompositeKey.equal(key1, key3); // false (nesting keys does not flatten them)
+```
+
+### Symbol.keyBy (follow on?)


In the last call we discussed what the proposal might look like not including a symbol protocol.

The reason for including the protocol would be to create a separation between the APIs that have this object based equality and the objects themselves. For example building a Set::<Temporal.PlainMonthDay>. The Set could be constructed with a generic call like Set.usingKeys(), and has no knowledge about Temporal.PlainMonthDay instances specifically; it only knows to call Symbol.keyBy and expect a CompositeKey (or primitive) in return. It is left to the Temporal.PlainMonthDay class to be responsible for providing a good implementation of the Symbol.keyBy method.

However a similar separation could also be achieved with an API like Set.usingKeyBy(Temporal.keyBy). Set on its own does not know about Temporal, and Temporal is responsible for providing the keyBy implementation. This API makes things very explicit. The main downside of this would be that it becomes harder to create Sets of a mixture of different domains. With the symbol protocol it would relatively straightforward to create a Set that contains a mixture of both Temporal and URL objects, (assuming they both implemented the protocol). However it might be that this type of use case is rarely required, and the majority of object based Sets would only need to contain a single domain of objects and this domain would be known at the point where it is constructed.

Another place where the Symbol protocol could be useful is with the Record and Tuple composite value discussed in this proposal. The generic protocol would allow other object types to participate in the built-in keyBy equality of the Record/Tuple. e.g. two Tuples containing two of the semantically same Temporal.PlainDate (maybe they mark the start and end dates of an event) objects, then the Symbol.keyBy of these two Tuples would be equal. The built-in Symbol.keyBy of a Record/Tuple would be there to ideally cover the vast majority of use-cases out-of-the-box without needing code authors to keep having to implement the protocol themselves. This is simpler to other languages that support record/tuple like values and have hash/equality protocols (e.g. Kotlin/Swift).

The downside of Record and Tuple looking up the Symbol.keyBy of their constituents is that this makes the Symbol.keyBy protocol Record and Tuple both more dynamic and observable. If Record and Tuple equality was limited to the the equality of privates and other Records and Tuples then much of how this was implemented exactly could be hidden, e.g. is it calculated eagerly on creation or lazily on demand. And it would be guaranteed to never change. If they look up the symbol then this can be hooked into to observe when the key is being generated, this means the spec would have to encode exactly when the values are looked up, and also if they are cached to avoid changing or called repeatedly risking a changing key. A different way to make it ergonomic to include non R/T values within a R/T and still produce a compositeKey for the structure could be with other helper functions (Object.recursvieKeyBy(obj, fn)) or maybe decorators.

Symbol protocols is how this sort of thing is done in the language - why would it be worth exploring an entirely unprecedented form of interop?

@ljharb I thought that @acutmore comment made a pretty good summary of the drawbacks of a symbol based approach, namely the intrinsic instability of an equality based on a dynamic deferral to the constituents.

That’s how all protocols work in the language.

Right, which is problematic for a feature that expresses equality. An instable mechanism has major issues for this specific use case, which warrants exploring alternative approaches.

Like Symbol.hasInstance, for example.

A protocol means you're trusting, implicitly, anything that conforms to the protocol.

acutmore · 2023-12-15T13:28:41Z

Thanks all for the input into this initial PR. I think it has served its purpose of allowing any part of the initial text to be directly commented on. I will merge this as is, so discussions can continue in dedicated issues and PRs.

add initial text

bed9fe1

nicolo-ribaudo reviewed Jun 27, 2023

View reviewed changes

acutmore added 4 commits June 27, 2023 15:18

add tests

6401ebb

README clarifications

c3bbbbc

apply suggestion from code review

4368697

expand Q&A

5e6e9ba

mhofman reviewed Jul 14, 2023

View reviewed changes

update with suggestions

694fc76

- CompositeKey construct no longer varargs - `keyBy` Map/Set config only accepts function - Add Map.usingKeys helper instead - Make `Symbol.keyBy` a method rather than a getter

mhofman reviewed Jul 21, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

mhofman reviewed Jul 21, 2023

View reviewed changes

shicks reviewed Jul 24, 2023

View reviewed changes

littledan approved these changes Jul 25, 2023

View reviewed changes

acutmore added 6 commits August 16, 2023 19:26

CompositeKey back to varargs

da82ab9

add Tuple to polyfill

58a39f0

polyfill: handle symbol keys

0ec45f9

remove CompositeKey.isKey - hide branch check in toStringTag

809743d

update README

584bab6

add Set polyfill

c2539da

ljharb reviewed Aug 16, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

mhofman reviewed Aug 16, 2023

View reviewed changes

acutmore and others added 2 commits August 17, 2023 13:59

README updates

8f6fd92

Apply suggestions from code review

827f514

Co-authored-by: Jordan Harband <ljharb@gmail.com>

acutmore commented Sep 2, 2023

View reviewed changes

acutmore merged commit f327715 into main Dec 15, 2023

acutmore deleted the init branch December 15, 2023 13:28

mhofman mentioned this pull request Dec 18, 2023

CompositeKey and WeakMaps #3

Open


		### Map and Set config (phase 1)

		Allow `Map` and `Set` instances to be customized with a lookup function that will produce the value that represents the keys values for that collection. In this mode the representative values are considered equal if they are `SameValueZero` equal, or if they are both `CompositeKey`s and equal according to `CompositeKey.equal`.

		- A `new CompositeKey(0, 0)` does not hold any lifetime information, a matching `CompositeKey` can always be created if the _inputs_ are available.
		- Allowing only `CompositeKeys` that were created from at least one value that itself is allowed as a `WeakMap` key could be an option.


		Immutable values types such as those in Temporal could implement `Symbol.keyBy`, without requiring users to work out the best way to represent these types using a `CompositeKey`.

		## Q+A

add initial text #1

add initial text #1

Conversation

acutmore commented Jun 26, 2023

nicolo-ribaudo 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

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

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

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

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

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

Choose a reason for hiding this comment

mhofman 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

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

acutmore commented Dec 15, 2023