-
Notifications
You must be signed in to change notification settings - Fork 247
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
[suggest-fix] Run test PRs on azure-docs-sdk-python-363 (part 1) #1931
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mimisasouvanh A couple of comments, prep for Thursday's call about these proposals.
@@ -32,7 +32,7 @@ constructor: | |||
description: 'URL to use for credential authentication with AAD. Its value could | |||
be | |||
|
|||
"[https://management.azure.com](https://management.azure.com)", "[https://management.chinacloudapi.cn](https://management.chinacloudapi.cn)" | |||
"`https://management.azure.com`", "[https://management.chinacloudapi.cn](https://management.chinacloudapi.cn)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These proposals (backtick wrapping and zapping management.azure.com links specifically) are drowning out any potentially interesting ones, can they be disabled and the tool rerun to see what the output looks like?
- These URLs are not actually links in the source, I believe the docs translator tool detects URLs in description fields and renders them as markdown links. For this small set of URLs that appears frequently, if the link-fixer is just going to remove them (and why is it only removing management.azure.com and not the others?), I think we'd be better off just not putting them in. Maybe we can detect and skip in the translator.
- Wrapping of URLs in backticks might be something we can do in the translator as well, at least for description fields (I don't think they get detected elsewhere, which is why they aren't Markdown links in fields like
syntax
anddefaultvalue
). The convention in the source appears to be to put them in double-quotes instead; having both together looks kind of funky and sort of defeats the point I think.
(Link to relevant translator source for posterity)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looping in @junshi356 for opinions/input on py2docfx.
Jun, the context here is that we are looking into running the automated link-checker on ref repos. We're aware that any changes committed here would be eliminated with the next pipeline run, this is just an experiment at this point.
Here in the python repo we see many instances of a couple of the same items (listed above). Would these be feasible changes in py2docfx?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, @nickwalkmsft sorry for the delay, the github notification didn't reach my MSFT mail. All API yaml documents in Python reference repos are purged then added again in each of the pipeline execution, so manual changes will be overwritten as long as the converter or the source package isn't updated. I think it's the right way to add the fix into py2docfx converter to make it persistent and valid for future documents.
@@ -8,7 +8,7 @@ inheritances: | |||
summary: Azure API Center Resource Provider. | |||
constructor: | |||
syntax: 'ApiCenterMgmtClient(credential: TokenCredential, subscription_id: str, | |||
base_url: str = ''https://management.azure.com'', **kwargs: Any)' | |||
base_url: str = ''`https://management.azure.com`'', **kwargs: Any)' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
syntax
gets rendered to code blocks, we don't want anything modified in these.
@@ -48,7 +48,8 @@ variables: | |||
- description: 'Specifies the tags that are assigned to the virtual machine. For more | |||
information | |||
|
|||
about using tags, see [Using tags to organize your Azure resources](https://docs.microsoft.com/azure/azure-resource-manager/resource-group-using-tags.md).' | |||
about using tags, see [Using tags to organize your Azure resources](https://docs.microsoft.com/azure/azure-resource-manager/resource-group-using-tags).' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MD fix
created-by-lianli