Skip to content

Conversation

carlossanlop
Copy link
Contributor

Area owners of Microsoft.Extensions.ObjectPool: @maryamariyan @ericstj
Note: The area owners document does not have the label area-Extensions-ObjectPool.

@opbld32

This comment has been minimized.

@ericstj ericstj changed the title Automatic port of Microsoft.Extensions.ObjectPool docs Automatic port of Microsoft.Extensions.Http.Polly docs Oct 1, 2020
@ericstj ericstj requested review from a team and removed request for maryamariyan and ericstj October 1, 2020 17:11
@ericstj
Copy link
Member

ericstj commented Oct 1, 2020

Title looks wrong. The problem pointed out is in PollyServiceCollectionExtensions. This appears to be a problem with the docs process as this type is extending a 3rd-party library and references it is complaining about are the types in that 3rd party component.

@ericstj
Copy link
Member

ericstj commented Oct 1, 2020

@carlossanlop I think @BillWagner tried to fix this here #4218 but that isn't working completely. Can someone more familiar with this tool try to fix this one?

@ericstj ericstj requested review from BillWagner and removed request for a team October 1, 2020 17:20
@ericstj ericstj changed the title Automatic port of Microsoft.Extensions.Http.Polly docs Automatic port of Microsoft.Extensions.ObjectPool docs Oct 1, 2020
@ericstj
Copy link
Member

ericstj commented Oct 1, 2020

Apologies, I misunderstood the purpose here. The validation warnings are due to the problem I mentioned above, which I do think @BillWagner should try to fix.

This is updating docs for Microsoft.Extensions.ObjectPool, which is part of ASP.NET -- @pranavkm @ryanbrandenburg

@ericstj ericstj removed the request for review from ryanbrandenburg October 1, 2020 17:40
@BillWagner
Copy link
Member

Thanks @ericstj

Adding @gewarren and @joelmartinez for the Polly based warnings. The XML files shouldn't have xrefs to the Polly docs. If so, we'll need to somehow get those removed because we don't publish the docs for those.

Pending that answer, this is ready to :shipit:

@BillWagner BillWagner requested a review from gewarren October 6, 2020 15:09
@joelmartinez
Copy link
Contributor

You could always put those polly XREFs in the missingapi.yml file, and point them to the correct external documentation page:
https://github.com/dotnet/dotnet-api-docs/blob/master/_zip/missingapi.yml

To be clear, this is a technical answer to the issue ... not sure how this is to be treated from an editorial perspective :)

<param name="obj">The object to return to the pool.</param>
<summary>Runs some processing when an object was returned to the pool. Can be used to reset the state of an object and indicate if the object should be returned to the pool.</summary>
<returns>
<see langword="true" /> if the object should be returned to the pool. <see langword="false" /> if it's not possible/desirable for the pool to keep the object.</returns>
Copy link

Choose a reason for hiding this comment

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

I realize this is what the existing comment says, but should this say:

Suggested change
<see langword="true" /> if the object should be returned to the pool. <see langword="false" /> if it's not possible/desirable for the pool to keep the object.</returns>
<see langword="true" /> if the object should be returned to the pool. <see langword="false" /> if it's not possible or desirable for the pool to keep the object.</returns>

Similarly elsewhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @pranavkm for providing a suggestion. It's ok to change the original texts from triple slash, since the purpose of the dotnet-api-docs PRs is to get them through language review, like you did here 😄 . I'll apply your suggestion in all the places where I find similar text.

@opbld30
Copy link

opbld30 commented Oct 6, 2020

Docs Build status updates of commit d0ef037:

✅ Validation status: passed

File Status Preview URL Details
xml/Microsoft.Extensions.ObjectPool/DefaultPooledObjectPolicy`1.xml ✅Succeeded View
xml/Microsoft.Extensions.ObjectPool/PooledObjectPolicy`1.xml ✅Succeeded View
xml/Microsoft.Extensions.ObjectPool/StringBuilderPooledObjectPolicy.xml ✅Succeeded View

For more details, please refer to the build report.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

For any questions, please:

@carlossanlop
Copy link
Contributor Author

@BillWagner @gewarren can I get an approving sign-off please?

Copy link
Member

@BillWagner BillWagner left a comment

Choose a reason for hiding this comment

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

This LGTM @carlossanlop @ericstj

You can :shipit: when ready.

@BillWagner
Copy link
Member

I opened #4952 which should fix the Polly related build warnings. Working with @joelmartinez to make sure that's setup correctly.

Co-authored-by: Genevieve Warren <24882762+gewarren@users.noreply.github.com>
@opbld34
Copy link

opbld34 commented Oct 14, 2020

Docs Build status updates of commit 8e34b7f:

✅ Validation status: passed

File Status Preview URL Details
xml/Microsoft.Extensions.ObjectPool/DefaultPooledObjectPolicy`1.xml ✅Succeeded View
xml/Microsoft.Extensions.ObjectPool/PooledObjectPolicy`1.xml ✅Succeeded View
xml/Microsoft.Extensions.ObjectPool/StringBuilderPooledObjectPolicy.xml ✅Succeeded View

For more details, please refer to the build report.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

For any questions, please:

@carlossanlop carlossanlop merged commit 0a7f84c into dotnet:master Oct 14, 2020
@carlossanlop carlossanlop deleted the M_E_ObjectPool branch October 14, 2020 06:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants