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

Added structural types and properties documentation properties #2236

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

henning-krause
Copy link

Issues

This PR adds documentation capabilities to the OdataConventionModelBuilder (as requested by #1732).

Description

The changes add new properties Description and LongDescription to StructualTypeConfiguration and PropertyConfiguration. Additionally, a new Attribute DescriptionAttribute is introduced to allow the developer to set the description via attributes on properties and classes.

Using this, one can now utilize the C# XML documentation (Using the DocXml - C# XML documentation reader) for the entities:

private static void OnModelCreating(ODataConventionModelBuilder builder)
{
	var reader = new DocXmlReader();
	
	foreach (var type in builder.StructuralTypes)
	{
		var comment = reader.GetTypeComments(type.ClrType);
		if (comment != null)
		{
			if (!string.IsNullOrEmpty(comment.Summary)) type.Description = comment.Summary;
			if (!string.IsNullOrEmpty(comment.Remarks)) type.LongDescription = comment.Remarks;
		}

		foreach (var property in type.Properties)
		{
			var propertyComment = reader.GetMemberComments(property.PropertyInfo);
			if (propertyComment != null)
			{
				if (!string.IsNullOrEmpty(propertyComment.Summary)) property.Description = propertyComment.Summary;
				if (!string.IsNullOrEmpty(propertyComment.Remarks)) property.LongDescription = propertyComment.Remarks;
			}
		}
	}
}

When the metadata is emitted, the proper Odata `Description` and `LongDescription` metadata annotations are included in the document.

Checklist (Uncheck if it is not completed)

  • Test cases added
  • Build and test with one-click build and test script passed

Additional work necessary

If the PR is accepted, I would add this functionality to functions and actions and their parameters.

@xuzhg
Copy link
Member

xuzhg commented Jul 23, 2020

@henning-krause Thanks for contribution. Just For your information, did you see "OData/ModelBuilder#9".
Can we combine your PR with the whole vocabulary configuration?

@henning-krause
Copy link
Author

@xuzhg By all means, do that. Can I help to speed things up?

@henning-krause
Copy link
Author

@xuzhg since OData/ModelBuilder#9 has been merged, is there a chance to merge this also? I don't see anything of my code in the referenced PR.

@henning-krause
Copy link
Author

@xuzhg Any news on this PR?

@mikepizzo
Copy link
Member

Hi @henning-krause -happy new year!

Thanks for your contribution, and sorry to have taken so long to follow up.

OData/ModelBuilder#9 provides the ability to implement patterns such as:

	var comment = reader.GetTypeComments(type.ClrType);
	if (comment != null)
	{
		if (!string.IsNullOrEmpty(comment.Summary)) type.HasDescription(comment.Summary);
		if (!string.IsNullOrEmpty(comment.Remarks)) type.HasLongDescription(comment.Remarks);
	}

Is this sufficient for what you're trying to do?

@henning-krause
Copy link
Author

Hi @mikepizzo,

I've just upgraded my solution to the latest odata libs (Microsoft.AspNet.OData 7.5.4 and Microsoft.OData.Core and Microsoft.OData.Edm 7.8.1) but I can't find the methods you're refering to.

I'm working with the ODataConventionModelBuilder and I don't see those methods, at least not on things like

  builder.EntitySet<Role>("Roles");

It seems that I'm missing something here.

@mikepizzo
Copy link
Member

It looks like OData/ModelBuilder#9 hasn't been backported to AspNet.OData 7.x. Let's try and do that first, so that developers using this feature in 7.x will have a consistent experience in 8.x.

@g2mula -- Can you look at porting your OData/ModelBuilder#9 PR to this branch?


if (edmType is IEdmStructuredType structuredType)
{
foreach (var entry in structuredType.DeclaredProperties.Join(structuralType.Properties, p => p.Name, p => p.Name, (property, configuration) => new {property, configuration}))
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand this correctly, I think that maybe ToDictionary is more appropriate than Join. Wouldn't this be equivalent functionality, but a bit more optimized?

var structuredTypeProperties = structuredType.DeclaredProperties.ToDictionary(p => p.Name, p => p);
foreach (var structuralTypeProperty in structuralType.Properties)
{
  if (!structuredTypeProperties .TryGetValue(structuralTypeProperty.Name, out var structuredTypeProperty)
  {
    continue;
  }

  if (!string.IsNullOrEmpty(structuralTypeProperty.Description)
  {
    model.SetDescriptionAnnotation(structuredTypeProperty, structuralTypeProperty.Description);
  }

  if (!string.IsNullOrEmpty(structuralTypeProperty.LongDescription)
  {
    model.SetDescriptionAnnotation(structuredTypeProperty, structuralTypeProperty.LongDescription);
  }
}

public string LongDescription
{
get => _longDescription;
set => _longDescription = value ?? throw Error.PropertyNull();
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that throwing is appropriate here because _longDescription will be null by default, so we aren't really protecting against anything

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