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

Support predefined class-, property and enum mappings #2820

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

ewoutkramer
Copy link
Member

@ewoutkramer ewoutkramer commented Jul 23, 2024

At this moment, the only way to initialize a ModelInspector is to reflect on the types for the POCOs. This is normally fast enough, but there are certainly other options, like pre-generating the metadata that is normally reflected, possibly at the same moment that you generate the POCO's. To enable this scenario, we have to open up the public surface of ModelInspector, ClassMapping, PropertyMapping and EnumMapping a bit, so source code generators are also able to initialize a ModelInspector.

This PR fixes #2794. Thanks to @almostchristian for bringing this usecase up, and working on its design.

@ewoutkramer
Copy link
Member Author

Ok, I have made a first version for @almostchristian to review. Note that I did not use required properties in the end, the few things that are required are still arguments to the constructor. In any case, all of that is now public/init.

I did not want to make the constructors on ClassMapping/PropertyMapping public, since I have other plans with them, but I created subclasses PocoClassMapping/PocoPropertyMapping/PocoEnumMapping that do, so that's what you should use. And maybe it will turn out we won't need it, then we can remove it again.

Anyway, Christian, if you're happy with it, we can pull it, and then there will be a new alpha release built automatically that you can test as well.

@mmsmits
Copy link
Member

mmsmits commented Aug 27, 2024

@almostchristian Is this something you are willing to take a look at?

@mmsmits mmsmits marked this pull request as ready for review August 27, 2024 08:19
@almostchristian
Copy link
Contributor

From what I see in the firely sdk codebase, no code seems to be using the PropertyMapping.NativeProperty field. Searching in Github, I see it is being used in the validator repo by @brianpos https://github.com/brianpos/Hl7.Fhir.FhirPath.Validator/blob/6684bdd8fc19626be05531b8d75eb60e2d7b757e/src/Hl7.Fhir.Base.FhirPath.Validator/BaseFhirPathExpressionVisitor.cs#L672

@mmsmits
Copy link
Member

mmsmits commented Sep 27, 2024

@almostchristian Do you mind reviewing this PR for us? Would really appreciate it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants