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

CBOR and platform check API updates #4992

Merged
merged 2 commits into from
Oct 16, 2020
Merged

Conversation

gewarren
Copy link
Contributor

Spell out CBOR acronym on first use.
Update platform check method descriptions.

(Hide whitespace changes in diff.)

@gewarren gewarren requested review from jozkee and buyaa-n October 15, 2020 00:45
@opbld31
Copy link

opbld31 commented Oct 15, 2020

Docs Build status updates of commit af9c153:

✅ Validation status: passed

File Status Preview URL Details
xml/System.Formats.Cbor/CborConformanceMode.xml ✅Succeeded View
xml/System.Formats.Cbor/CborContentException.xml ✅Succeeded View
xml/System.Formats.Cbor/CborReader.xml ✅Succeeded View
xml/System.Formats.Cbor/CborSimpleValue.xml ✅Succeeded View
xml/System.Formats.Cbor/CborTag.xml ✅Succeeded View
xml/System.Formats.Cbor/CborWriter.xml ✅Succeeded View
xml/System/OperatingSystem.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:

Copy link
Contributor

@buyaa-n buyaa-n left a comment

Choose a reason for hiding this comment

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

OperatingSystem type-related changes looks good, thanks

@jozkee jozkee requested a review from eiriktsarpalis October 16, 2020 05:31
@@ -20,7 +20,7 @@
</Attribute>
</Attributes>
<Docs>
<summary>To be added.</summary>
<summary>The exception that is thrown when Concise Binary Object Representation (CBOR) content is invalid.</summary>
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to do this for every type? It feels redundant since the namespace also contains the acronym. I would suggest expanding it only for the CborWriter and CborReader classes, since they are the entrypoints to this particular API.

Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

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

Other than one comment, CBOR changes LGTM

@@ -1,6 +1,6 @@
<Namespace Name="System.Formats.Cbor">
<Docs>
<summary>To be added.</summary>
<summary>Contains types used in reading and writing data in the Concise Binary Object Representation (CBOR) format.</summary>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eiriktsarpalis Thanks for reviewing. I removed the spelled out acronym from all but reader and writer, but I also added a namespace summary here. Does this look okay?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

That's perfect, thanks!

@opbld34
Copy link

opbld34 commented Oct 16, 2020

Docs Build status updates of commit 59584ba:

✅ Validation status: passed

File Status Preview URL Details
xml/System.Formats.Cbor/CborContentException.xml ✅Succeeded View
xml/System.Formats.Cbor/CborReader.xml ✅Succeeded View
xml/System.Formats.Cbor/CborWriter.xml ✅Succeeded View
xml/System/OperatingSystem.xml ✅Succeeded View
xml/ns-System.Formats.Cbor.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:

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.

5 participants