Convert JsonLDContextObject, JsonLDTerm, and JsonLDExpandedTerm to record classes about activitypubsharp HOT 17 CLOSED

Is the goal for this to be able to compare both objects by property value equality instead of by reference? I was planning on picking this one up, but saw that the Equals() stuff were using ReferenceEquals() so just want to make sure the plan was to switch to value comparisons since that's what record will do by default (for value types).

from activitypubsharp.

@warriordog No problem! And sure. I'll try doing some more research as well to see if there are any better alternatives. The gist seems decent but it would add some bloat 😅

from activitypubsharp.

Sure that should work!

from activitypubsharp.

I am happy to take this but I may not have the time to get to it until next week. If you have the bandwidth to take it before then then you can feel free to!

from activitypubsharp.

@CreggHancock sounds good! I may do it before then depending on my schedule this week. I'm getting very close to completing Project 1 (AS extensions) and this is a part of that.

from activitypubsharp.

Done. This should be the last thing before merging extensions support

from activitypubsharp.

@CreggHancock Yes, that's the intent. These are meant to be immutable and unique, so giving them value equality allows them to be used with HashSet and Dictionary without any duplication. Thanks for confirming!

from activitypubsharp.

Yep totally makes sense! I still feel like we'll run into an issue with the dictionary in JsonLDContextObject with it being a reference type. Some quick investigation shows it's trying to compare equality with reference type properties by reference even after JsonLDContextObject has been converted to a record.

I have found some possible solutions to this, like rolling out your own dictionary class that handles value comparison for equality. An example is in this gist. If that's a route you'd like to go down I'm happy to implement that.

from activitypubsharp.

@CreggHancock sorry, I totally missed your last comment! Thank you for bringing that up. Let me do some thinking and I'll get back to you.

from activitypubsharp.

@CreggHancock Do other collections, like List<T>, work for value equality? I just realized that the list of contexts is small enough that we can probably do a naive linear search with no noticeable performance impact.

from activitypubsharp.

@CreggHancock Related, we could probably just implement a basic linked list for this. I've never seen a real-world object with more than 5 context entries, or more than 15 key/value pairs in a single object.

from activitypubsharp.

@warriordog Yeah if we implement our own list that overrides List that we can use throughout the project and it implements its own equality contract that is value based, that would solve the issue.

from activitypubsharp.

@CreggHancock we should only need to use it in the context types, right? We don't want everything to be value-typed.

from activitypubsharp.

Sure! The intention would be to use it for comparing these record types. I just meant it could be used throughout the project if we need value based comparisons. If we don't want that I can just implement it as an internal class though

from activitypubsharp.

@CreggHancock do you think we would need this anywhere else? I can't think of any other place that needs to compare a list by value equality.

from activitypubsharp.

@CreggHancock I think the best option to to implement a custom collection. For safety, I think it should not be generic and instead be hardcoded to only the exact case we need (IReadOnlyDictionary<string, JsonLDTerm>). Otherwise, someone could accidentally use it with a mutable type. What do you think?

from activitypubsharp.

@CreggHancock just to clarify - did you want to pick up this work, or should I go ahead and do it?

from activitypubsharp.