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

feat: expose TypeResolver and TypeConverts #80

Merged
merged 2 commits into from
Oct 9, 2023

Conversation

patburke234
Copy link
Contributor

At NCQA, our existing DCS software utilizes the TypeResolver and TypeConverter classes. We use these to convert between the CQL and FHIR data models in various parts of our application. This PR is to expose the minimum set of classes/methods that we need publicly.

@@ -18,8 +18,10 @@

namespace Hl7.Cql.Fhir
{
internal static class FhirTypeConverter
public static class FhirTypeConverter
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove the

#pragma warning disable CS1591 // Missing XML comment for publicly visible type or member

from the top of the file and document this class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I will update it in a subsequent PR.

@@ -13,9 +13,9 @@
using Hl7.Fhir.Model;
namespace Hl7.Cql.Packaging
{
internal class CqlCrosswalk
public class CqlCrosswalk
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove the

#pragma warning disable CS1591 // Missing XML comment for publicly visible type or member

from the top of the file and document this class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I will update it in a subsequent PR.

@@ -247,7 +247,7 @@ public CqlCrosswalk(TypeResolver typeResolver)
}
}

internal class TypeEntry
public class TypeEntry
Copy link
Member

Choose a reason for hiding this comment

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

This feels like a record. We should remove the setters for the properties. Maybe we can even make this a record class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, this could definitely be a record type. I will update it in a subsequent PR.

@@ -13,9 +13,9 @@
using Hl7.Fhir.Model;
namespace Hl7.Cql.Packaging
{
internal class CqlCrosswalk
public class CqlCrosswalk
Copy link
Member

Choose a reason for hiding this comment

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

These classes do feel like internal utility classes to me. What are they? A kind of mapping from a FHIR type to a CQL type? Should this also work for other models?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's a mapping between FHIR and CQL types. We use it to convert our generated outputs to the data model our customers want (FHIR or CQL). I could see it expanding to other data models in the future, but I would think we'd need create a parent and rename this "FhirCqlCrosswalk", which is what it used to be named.

@ewoutkramer ewoutkramer merged commit 3f12cd8 into FirelyTeam:develop Oct 9, 2023
2 checks passed
@ewoutkramer
Copy link
Member

ewoutkramer commented Oct 10, 2023

Oops, I didn't mean to merge it yet ;-) Now we need another PR to fix my remarks. Don't know what happened there...

Interesting, I didn't. It's an automated message, I requested changes, but by doing so, I fulfilled some requirement and it got merged automatically. Mmmmm.....

@patburke234
Copy link
Contributor Author

Oops, I didn't mean to merge it yet ;-) Now we need another PR to fix my remarks. Don't know what happened there...

Interesting, I didn't. It's an automated message, I requested changes, but by doing so, I fulfilled some requirement and it got merged automatically. Mmmmm.....

Very strange. I'll work on a PR soon to get these items fixed.

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