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

Improve generated description from xml docs #1473

Conversation

kiapanahi
Copy link
Contributor

@kiapanahi kiapanahi commented Dec 20, 2023

Addresses: #1428

This PR uses a compiled regex to find and replace all namespaced types/methods/properties in a summary section in components' XML docs.

Microsoft Reviewers: Open in CodeFlow

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-integrations Issues pertaining to Aspire Integrations packages label Dec 20, 2023
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Dec 20, 2023
Copy link
Member

@drewnoakes drewnoakes left a comment

Choose a reason for hiding this comment

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

I have only a comment on the implementation. I'll leave it to others to comment on the general direction of this change.

@sebastienros
Copy link
Member

Would it be simpler if the logic was integrated in StripXmlElements directly by checking for well-known tags (<see cref, <see langword) and falling back to a default logic (taking the first argument). This way you can "just" check to the colon and then skip the first two chars.
It would not need any Regex and the false positives that @drewnoakes mentioned.

@kiapanahi
Copy link
Contributor Author

Would it be simpler if the logic was integrated in StripXmlElements directly by checking for well-known tags (<see cref, <see langword) and falling back to a default logic (taking the first argument). This way you can "just" check to the colon and then skip the first two chars. It would not need any Regex and the false positives that @drewnoakes mentioned.

The latest commit addresses those missing regex issues @drewnoakes mentioned. However, going down the path of changing the StripXmlElements() method and checking against known tags and attributes would AFAIK result in having the text of an attribute that might not always contain their full names. For example:

KeyVaultSecretIdentifier instead of Azure.Security.KeyVault.Secrets.KeyVaultSecretIdentifier.

Or did I misunderstand you?

@sebastienros
Copy link
Member

sebastienros commented Dec 22, 2023

My comment was about not using a Regex at all. Trying to find these occurrences could be prone to errors if the content doesn't match for any reason we can't think of right now. Instead, just process the value when we detect it in StripXmlElements, so the line that take the first attribute in the tags will instead format the value and return it. That will remove the need for a Regex, and it will also let us return custom formatted values ('{0}', "{0}", links, ...) to the documentation if we want instead of applying a generic '{0}'.

@kiapanahi
Copy link
Contributor Author

My comment was about not using a Regex at all. Trying to find these occurrences could be prone to errors if the content doesn't match for any reason we can't think of right now. Instead, just process the value when we detect it in StripXmlElements, so the line that take the first attribute in the tags will instead format the value and return it. That will remove the need for a Regex, and it will also let us return custom formatted values ('{0}', "{0}", links, ...) to the documentation if we want instead of applying a generic '{0}'.

Sorry for the delay.
After a few hours of fiddling around, I added a simple(-ish) implementation of your suggestion; essentially moving all the "formatting" logic into StripXmlElements method altogether.

@sebastienros sebastienros merged commit 87fcaca into dotnet:main Dec 27, 2023
8 checks passed
@kiapanahi kiapanahi deleted the issue_1428/improve-descs-in-configuration-schema-generator branch December 27, 2023 20:25
@github-actions github-actions bot locked and limited conversation to collaborators Apr 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-integrations Issues pertaining to Aspire Integrations packages community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants