Process inline comments with DictionaryDeserializer. #865
Conversation
| /// </summary> | ||
| /// <returns>Returns true if there are more events available, otherwise returns false.</returns> | ||
| bool MoveNext(); | ||
|
|
There was a problem hiding this comment.
I'm very hesitant with adding additional fields/methods to this interface. It's implemented by other consumers of the library and I really, really badly don't want to break those. That's a major breaking change.
|
|
||
| namespace YamlDotNet.Serialization.NodeDeserializers | ||
| { | ||
| internal class CommentNodeDeserializer : INodeDeserializer |
There was a problem hiding this comment.
This should be public so people can remove it if needed.
| /// </summary> | ||
| public ParsingEvent? Current => current?.Value; | ||
|
|
||
| public bool SkipComments { get; } |
There was a problem hiding this comment.
This should be true by default, for sure. I'm not sure of what kind of repercussions this could have in places that inherit or use this outside of the library, I'm suspecting it could cause issues if dictionaries suddenly have additional unexpected objects.
I would like it to be disabled through these changes by default, and have to be opted into for it to work.
| { typeof(CollectionNodeDeserializer), _ => new CollectionNodeDeserializer(objectFactory.Value) }, | ||
| { typeof(EnumerableNodeDeserializer), _ => new EnumerableNodeDeserializer() }, | ||
| { typeof(ObjectNodeDeserializer), _ => new ObjectNodeDeserializer(objectFactory.Value, BuildTypeInspector(), ignoreUnmatched, duplicateKeyChecking, typeConverter) } | ||
| { typeof(CommentNodeDeserializer), _ => new CommentNodeDeserializer() }, |
There was a problem hiding this comment.
This should be optional and not included by default. You'll also need to add this to the staticdeserializerbuilder for feature parity with the ahead of time compilation abilities of the library.
| { typeof(EnumerableNodeDeserializer), _ => new EnumerableNodeDeserializer() }, | ||
| { typeof(ObjectNodeDeserializer), _ => new ObjectNodeDeserializer(objectFactory.Value, BuildTypeInspector(), ignoreUnmatched, duplicateKeyChecking, typeConverter) } | ||
| { typeof(CommentNodeDeserializer), _ => new CommentNodeDeserializer() }, | ||
| { typeof(ObjectNodeDeserializer), _ => new ObjectNodeDeserializer(objectFactory.Value, BuildTypeInspector(), ignoreUnmatched, duplicateKeyChecking, typeConverter) }, |
There was a problem hiding this comment.
Theres an extra comma on the end here.
| // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE | ||
| // SOFTWARE. | ||
|
|
||
| namespace YamlDotNet.Basic |
There was a problem hiding this comment.
Lets not create a new namespace, there should be one that exists already that we can put this in.
| /// <summary> | ||
| /// Gets the SkipComments setting. | ||
| /// </summary> | ||
| bool SkipComments { get; } |
There was a problem hiding this comment.
Same comment as in the IParser, I'm very hesitant to modify these core interfaces.
|
I'll be honest, I'm pretty hesitant with merging this in. Comments in the YAML spec (which we adhere to) are supposed to be ignored from my understanding. If they're really needed, I suspect a custom/modified node deserializer should be able to handle it without modifying any of the underlying parser/scanners. Suddenly changing/adding dictionaries or objects to dictionaries could have large unintended side effects with current consumers of the library, and that's something I really want to avoid at all costs. Let me think about this some more, and I'll get back to you. |
|
Hello, |
2 participants
Dear Antoine, Edward,
We need to process some data from comments in yaml files. In these comments we usually got some descriptions about data types, ranges, available values.
So I've tried to use your solution to process the yaml files, and created a CommentNodeDeserializer to process all simple values into a ValueWithComment pair and then use it in the DictionaryDeserializer. I set this behavior only if the skipComments is set to false.
I hope you can accept my solution, as we really need this feature to process our input files.
Best regards,
and many thanks,
Eva.
PS: I closed the previous pull request, because I had it on my master branch, and the appVeyor build failed when trying to get a version number.