allowing for extra properties in edi file declaration

[jose-sherpa]

EDI can be very obscure. In order to boost readability, we need to allow for extra unused fields like comments so that anyone can come in and read the file declaration.

For example given the following field:

{ "name": "N101", "index": 1 }

it would be nice to add a comment in order to give more context:

{ "name": "N101", "index": 1, "_comment": "name" }

[jf-tech]

@jose-sherpa first of all, thank you so much for contributing, and for this specific case, I completely agree with the idea.

However, in general, we're very strict on the schema to avoid any unintended consequences, thus we rarely allow "additionalProperties: true". For this "_comment", I think you should model it after this:

https://github.com/jf-tech/omniparser/blob/master/extensions/omniv21/validation/transformDeclarations.json#L148

Let me know if you have any questions in how to implementing this.

[jose-sherpa]

@jose-sherpa first of all, thank you so much for contributing, and for this specific case, I completely agree with the idea.

However, in general, we're very strict on the schema to avoid any unintended consequences, thus we rarely allow "additionalProperties: true". For this "_comment", I think you should model it after this:

https://github.com/jf-tech/omniparser/blob/master/extensions/omniv21/validation/transformDeclarations.json#L148

Let me know if you have any questions in how to implementing this.

Oh I didn't even see that, thank you I'll make the change!

[jose-sherpa]

@jf-tech actually what do you think about making _comments which is an array of strings? I am just thinking in some cases it might be useful to document enum value mappings like the following ISA05 element:

Interchange ID Qualifier
--
01 -  Duns (Dun & Bradstreet)
14 - Duns Plus Suffix
20 - Health Industry No (HIN)
27 - Carrier ID  assigned by HCFA
28 - Fiscal Intermediary ID assigned by HCFA
29 - Medical Provider & Supplier ID assigned by HFCA
30 - US Fed Tax ID
33 - NAIC Code
ZZ - Mutually defined

[jf-tech]

Looks good.

[codecov]

Codecov Report

Patch and project coverage have no change.

Comparison is base (b4d60ad) 100.00% compared to head (e771271) 100.00%.

Additional details and impacted files

@@            Coverage Diff            @@
##            master      #207   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           53        53           
  Lines         3021      3021           
=========================================
  Hits          3021      3021           

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[jf-tech]

@jf-tech actually what do you think about making _comments which is an array of strings? I am just thinking in some cases it might be useful to document enum value mappings like the following ISA05 element:

Interchange ID Qualifier
--
01 -  Duns (Dun & Bradstreet)
14 - Duns Plus Suffix
20 - Health Industry No (HIN)
27 - Carrier ID  assigned by HCFA
28 - Fiscal Intermediary ID assigned by HCFA
29 - Medical Provider & Supplier ID assigned by HFCA
30 - US Fed Tax ID
33 - NAIC Code
ZZ - Mutually defined

@jose-sherpa we have a rule of making schema easier to write for simple/majority cases, while possible for harder cases. So for vast majority cases here, I suspect a single line comment is good enough. For such scenario, adding a [] json array for a single comment string doesn't make sense. Now if multi-line comment scenarios are coming frequently and we really want to address it, we can either 1) introduce a new thing called _comments ('s' indicative of multiple lines) in the schema, in addition to the _comment you added here, or 2) move the schema JSON format to, say, https://github.com/hjson/hjson-go, where multi-line string is supported.

We thought about 2) for a while, but eventually decided not pursuing it, because the benefits or demands aren't significant enough to justify the deviation from built-in json format. We can revisit the decision, if needed.

I'll approve this PR. How about for now, you can use the the latest from master for your project for a while -- I'm a bit hesitant to cut a new release just for this. But do let me know if you have different requirements that warrant a new release.

[jose-sherpa]

@jf-tech I think this is good for now and I agree that I don't think this warrants a release, we can use master. I appreciate your responsiveness, it's always a good sign when the maintainer is as responsive as you are. Thanks again and I will open a PR if I find anything else during our implementation but so far the app does exactly what we need. Great work on this!

[jf-tech]

@jose-sherpa one last shameless plug :) do consider sponsoring, at whatever frequency or one-time, and whatever the amount, always appreciated!

[jose-sherpa]

@jf-tech , of course, I'll pass this along to the powers that be! This is a great project