Skip to content

Conversation

@mgramigna
Copy link
Contributor

Summary

Fill out the rest of the JSON schema for CSV config

New behavior

N/A

Code changes

Add all properties and some required ones to JSON Schema

Testing guidance

here is a URL with the updated JSON schema loaded into the validator.

Play around with this, paste your config in, delete some properties, invalid data type stuff, etc.

A visual inspection of the schema is also a good idea, it's not too dense

@Dtphelan1
Copy link
Contributor

DP2 - I think we could also use a commonExtractorArgs that defines an object with a baseFhirUrl property of type string with and format hostname

@Dtphelan1 Dtphelan1 self-assigned this Jun 21, 2021
@mgramigna
Copy link
Contributor Author

DP2 - I think we could also use a commonExtractorArgs that defines an object with a baseFhirUrl property of type string with and format hostname

Clarifying Q: we make use of commonExtractorArgs with the URL properties (including auth and such) in other "proprietary" versions of the mcode-extraction-framework. Does that property also apply to CSV config? Since the schema I made is pretty specific to that.

@mgramigna
Copy link
Contributor Author

DP2 - I think we could also use a commonExtractorArgs that defines an object with a baseFhirUrl property of type string with and format hostname

Clarifying Q: we make use of commonExtractorArgs with the URL properties (including auth and such) in other "proprietary" versions of the mcode-extraction-framework. Does that property also apply to CSV config? Since the schema I made is pretty specific to that.

Kind of answered my own question. Looks like we do parse/use commonExtractorArgs in the base apps so I will include a simple version of that in the schema

@jafeltra jafeltra self-assigned this Jun 21, 2021
Copy link
Contributor

@jafeltra jafeltra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good! I had a couple minor comments and a couple general questions.

Copy link
Contributor

@jafeltra jafeltra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me! All my questions were addressed or made into jira tasks. I'm approving since I'll be out tomorrow, but happy to be a part of any conversations about the comments from Dylan on Thursday if you wait until then to discuss!

Copy link
Contributor

@Dtphelan1 Dtphelan1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After responding to my comments and addressing Jules' comments, this looks good to me. Great work Matt 😄

@Dtphelan1 Dtphelan1 merged commit d5e3013 into develop Jun 23, 2021
@Dtphelan1 Dtphelan1 deleted the config-schema branch June 23, 2021 19:22
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