Skip to content

Conversation

@cipriancraciun
Copy link
Contributor

@cipriancraciun cipriancraciun commented Oct 23, 2018

This small patch add support for a new JS type Null which accepts the following values as JSON null:

  • the empty string;
  • the null and nil strings;
  • any case-variant of the above;

(Implements #20)

…strings, `null`, `nil` and any case-variation
@basgys
Copy link
Owner

basgys commented Oct 31, 2018

XML has a way to describe a null value, which is xsi:nill. e.g. <book xsi:nil="true" />

In some cases, an empty string can be considered an absence of value, but not always. Therefore, I would suggest to be more conservative and only include xsi:nil and maybe null / nil

@cipriancraciun
Copy link
Contributor Author

In some cases, an empty string can be considered an absence of value, but not always. Therefore, I would suggest to be more conservative and only include xsi:nil and maybe null / nil

Given your suggestion and my initial use-case I think there should be multiple "null" converters:

  • the xsi:nil you've proposed;
  • the null / nil textual variants;
  • the empty string;

I understand that your library tries to be as close as possible to the original XML format, and therefore an empty tag should be considered as an empty string. However my initial use-case dealt with XML files that were generated by a C application where a NULL value was formatted as an empty string.

As such would you accept a patch that introduces the later two "null" converters? Say NullFromStringNull and NullFromStringEmpty?

@basgys
Copy link
Owner

basgys commented Nov 1, 2018

Yes. As I mentioned, an empty string can be considered as an absence of value in some cases.

For that reason, I am definitely in favour of having the option to tell the converter whether an empty string should be considered an absence of value or not. (aka null, nil, NULL, ...).

Now the question is, can we apply this rule only to specific nodes? It would be the ideal solution imo.

@cipriancraciun
Copy link
Contributor Author

Now the question is, can we apply this rule only to specific nodes? It would be the ideal solution imo.

Personally I think that if one needs this level of configuration, then perhaps handling the XML with an native API would be the best choice.

I wonder how people use this library, as this might better inform the decision. My assumption is that one uses such a library as a quick solution to ingesting XML and dumping it into an JSON-compliant data-store. As such in these situations a "loose" translation is acceptable.

@basgys
Copy link
Owner

basgys commented Nov 1, 2018

... if one needs this level of configuration, then perhaps handling the XML with an native API would be the best choice.

Fair point. We can start with a config that applies to the whole document and improve it down the road when we see fit.

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.

2 participants