Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Inconsistency in auto-generated Markdown properties? #495

Closed
michelemottini opened this issue Dec 4, 2017 · 13 comments
Closed

Inconsistency in auto-generated Markdown properties? #495

michelemottini opened this issue Dec 4, 2017 · 13 comments
Assignees

Comments

@michelemottini
Copy link
Contributor

The markdown properties in https://github.com/ewoutkramer/fhir-net-api/blob/develop-stu3/src/Hl7.Fhir.Core/Model/Generated/StructureDefinition.cs are declared as

public Hl7.Fhir.Model.Markdown Xxxx { . . . }

whereas those in https://github.com/ewoutkramer/fhir-net-api/blob/develop-stu3/src/Hl7.Fhir.Core/Model/Generated/ElementDefinition.cs are declared as

public Hl7.Fhir.Model.Markdown XxxxElement { . . . }

public string Xxxx { . . . }

that is more consistent with other primitive type (FhirString etc) properties

@ewoutkramer
Copy link
Member

@brianpos looks irregular, yet nothing here is done manually....any ideas?

@brianpos
Copy link
Collaborator

I'll take a look

@ewoutkramer
Copy link
Member

@brianpos: Yes, please ;-)

@ewoutkramer ewoutkramer added this to the Cologne 2018 (0.96) milestone Apr 12, 2018
@brianpos
Copy link
Collaborator

This still the case with the R4, or is it new to R4?

@michelemottini
Copy link
Contributor Author

Seen only in STU3, did not try R4

@ewoutkramer ewoutkramer removed this from the Cologne 2018 (0.96) milestone Jun 6, 2018
@ewoutkramer
Copy link
Member

@brianpos found out what the issue is while refactoring the .tt files for R4.

There was an inconsistency in generating the Markdown property: both the xxxxElement and xxx were of type Markdown, we should now split it up into xxxElement:Markdown and xxxx:string.

Note: this is a breaking change and it changes the IConformanceResource interface too.

Will discuss this breaking change at the next community meeting on monday.

@brianpos
Copy link
Collaborator

brianpos commented Dec 8, 2018

While updating my R4 server, I'm wanting us to go back to not generating the string convenience property, and force the users to handle the Markdown class.
Its made it to hard to tell what is a markdown string vs just a normal string.
cs.Description = "Description of the codesystem";
Is this setting a string or a markdown value?
vs cs.Description = new Markdown("Description of the codesystem");

@ewoutkramer
Copy link
Member

At the .NET standup we agreed with Brian - @brianpos has now implemented this.

@brianpos
Copy link
Collaborator

But only in R4, was that the answer in the end?

@wmrutten
Copy link
Contributor

I'd prefer if Markdown properties would also provide ...Element mirror properties, similar to all other datatypes.
In Forge, all the different property accessor logic follows a similar pattern. Logic dealing with Markdown properties now clearly stands out and deviates from the pattern. For me, this counter-intuitive. I've added some ugly code comments to remind myself about why this is necessary. But I'd prefer an API design that follows a common pattern.

@brianpos
Copy link
Collaborator

That pattern is only used when a. Net native type is used for the property type, and the element one caters for the string value.
This one the native type could be string, but it isn't, it's markdown, and you should have special handling for this.

@wmrutten
Copy link
Contributor

That pattern is only used when a. Net native type is used for the property type, and the element one caters for the string value.

But isn't it actually the other way around? For example:

  • UrlElement : FhirUri

  • Url : string

  • KeyElement : Id

  • Key : string

Hence I would also expect:

  • CommentElement : Markdown
  • Comment : string

This one the native type could be string, but it isn't, it's markdown, and you should have special handling for this.

Not necessarily - you can treat any markdown value as a regular string. User doesn't get a nice formatted rendering, but it won't break your code.
Also Markdown datatype doesn't have any relevant child properties that clients would need to access, other than string Value.

Also, I don't see why Markdown should be considered special in this regard. Same argument holds for e.g. PositiveInt or Canonical and we don't treat these differently?

@wmrutten
Copy link
Contributor

Note: I can live with this irregularity, not saying we should re-open this issue. Just wondering about the motivation.

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

No branches or pull requests

4 participants