More robust data structures - #6
Merged
Merged
Conversation
…ips; implemented IDictionary<object, object> interface on NullKeyDictionary to expose more methods that will minimize the impact that switching from old Transit to new Transit.Net will have
oconnor0
approved these changes
Jul 8, 2026
oconnor0
reviewed
Jul 8, 2026
| public object Current => Entry; | ||
|
|
||
| KeyValuePair<object?, object?> IEnumerator<KeyValuePair<object?, object?>>.Current => _onNull | ||
| ? new KeyValuePair<object?, object?>(null!, _dict._nullValue) |
There was a problem hiding this comment.
Why null! when you're producing a value of object??
Author
There was a problem hiding this comment.
Ah, whoops. This was copied verbatim from a few lines above, where we create a DictionaryEntry object for similar reasons, and the signature of the DictionaryEntry constructor takes a non-null object for a key
There was a problem hiding this comment.
Ah, OK, was hoping it wasn't some required C# thing because of the context or generic arguments or something.
Member
|
It looks ok, but I see a test fails: |
|
Yeah, that's why #5 is still unmerged too. |
Member
|
Ok. To answer the question more generally, no need to wait for me when you're ready to merge on this stuff. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In order to smooth over transitions from the old Transit framework to the new one, some changes have been implemented for data structures.
NullKeyDictionarynow implementsIDictionary<object, object>andIReadOnlyDictionary<object, object>to make it more similar to the type it replaces from the old Transit,System.Collections.Immutable.ImmutableDictionary<object, object>. Callsites that originally used the old Transit expected to be able to use dictionary-related methods and extensions that are not all available from theSystem.Collections.IDictionaryinterface. This allows said callsites to simply change the type they cast values read from Transit fromImmutableDictionary<object, object>toIDictionary<object, object>without needing to change any other logic.A wrapper class
ListWrapperhas also been added to preserve serialization information. Lists we receive from environments like Clojure would normally most closely be represented by LinkedLists in C#, but we prefer to use List for performance. Therefore, this wrapper class is used so that if we need to roundtrip data back to the original environment, the original type of the list is maintained in the data we send. Similarly to the situation with dictionaries, the old Transit read lists asImmutableList<object>, and so for simplicity and to minimize impact on callsites, the wrapper class inherits fromList<object>.