Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get the yaml mapping node to preserve the order of the keys #468

Closed
wants to merge 10 commits into from
Closed

Get the yaml mapping node to preserve the order of the keys #468

wants to merge 10 commits into from

Conversation

a2937
Copy link
Contributor

@a2937 a2937 commented Feb 17, 2020

For this pull request, I am using a wrapper of the OrderedDictionary class to hold and manage all of the data stored in the YamlMappingNode. This wrapper was implemented because the OrderedDictionary class was unfortunately not generic , and I could not find a Generic version of the class in the System.Collections.Generic namespace.

The only problem I am having is that the OnDeserialization method was called while the object was not being deserialized. It keeps occuring in two test cases. I'm in the process of working around it. More than likely has to do with OrderedDictionary using a hashtable as an underlying data structure.
Thank you stack overflow for having something similar to this topic. Now the only problem is I can't actually tell the difference between the expected output and the actual output.
@aaubry
Copy link
Owner

aaubry commented Feb 17, 2020

I've checked your implementation and noticed that many of the IDictionary members are not implemented. This is a problem because users of the code may rely on those members. Maybe it would be simpler to use both a Dictionary<YamlNode, YamlNode> and a List<YamlNode> internally. This would also have the advantage of avoiding taking a new dependency on System.Collections.Specialized.
What do you think ?

Everything has been properly implemented now; and the tests are working finally.
@a2937
Copy link
Contributor Author

a2937 commented Feb 18, 2020

I implemented more of the IDictionary functions and it actually resolved the serialization issues. Funny how that works. The only downside is that the library will not be supported by Dotnet Standard 1.3. I've been trying to find another collection to use to serialize with instead; however I have found no such luck. I am still insisting on using the OrderedDictionary class. It is the only class that I am assured will keep the order of the key value insertions.

This method call is not actually necessary and should allow it to build on DotNetStandard Version 1.3.
Okay that should be everything needed for compatiblity.
@aaubry
Copy link
Owner

aaubry commented Feb 28, 2020

I'm sorry, I was unable to get back to you earlier. I'd rather avoid System.Collections.Specialized. This forces that dependency to anyone using the library while providing limited value in this case. If you look at the implementation of OrderedDictionary, you'll see that it consists of a dictionary and a list. Methods that add or remove add to both collections and enumeration iterates the list.
If you use this approach you will also benefit from generics which will avoid any casts.

Regarding serialization, that's not a use case that we intend to keep supporting. These tests are obsolete and I intended to remove them earlier but forgot about it. You may delete BinarySerlializationTests.cs and ignore serialization.

For some reason or another the YamlNode is being loaded back as a YamlAliasNode instead of the base class YamlNode. I can't figure out why.
@a2937
Copy link
Contributor Author

a2937 commented Mar 2, 2020

Okay I swapped to a Dictionary and List but can't quite figure out how to keep them from being loaded back as YamlNodes instead of YamlAliasNodes. Is it possible that I removed a library I shouldn't have or am using something incorrectly?

@aaubry
Copy link
Owner

aaubry commented Mar 5, 2020

I am confused about your last question. Can you explain ?

@a2937
Copy link
Contributor Author

a2937 commented Mar 5, 2020

I'm suddenly getting this error and don't know how to fix it or why it is happening : "System.NotSupportedException : A YamlAliasNode is an implementation detail and should never be visited."

Update: It's the enumerator on my collection. Perhaps we need to check the non-generic one as well to make it sure it will function properly.

Somehow enumerating the list manually instead of just retrieving the list's enumerator worked just fine.  I wonder if this means the non-generic enumerator won't work.
@airbreather
Copy link
Contributor

I was about to open an issue requesting this, but I see that there's already this PR for it, so I'll just comment here.

+1 on strengthening the YamlMappingNode contract to preserve the order of keys; the workaround I've found is to use a sequence of 1-element mapping nodes, which is clunky and doesn't automatically enforce uniqueness.

I've looked at this, and while there are a few things I'd do slightly differently here and there, the only thing I'm going to suggest at this time (as an outsider) is to change OrderedYamlDictionary accessibility from public to internal. With public, people might start using it for their own needs, which could complicate maintenance efforts in the future.

@airbreather
Copy link
Contributor

Ahh, was this actually done in #542?

@aaubry aaubry force-pushed the master branch 2 times, most recently from d4fd04a to cb0768c Compare March 30, 2021 12:15
@aaubry aaubry mentioned this pull request Apr 29, 2022
@EdwardCooke
Copy link
Collaborator

This was already implemented in another PR, closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants