Skip to content

Adding documentation for System.Text APIs tergeted for 3.0 #2875

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Aug 5, 2019
Merged

Adding documentation for System.Text APIs tergeted for 3.0 #2875

merged 3 commits into from
Aug 5, 2019

Conversation

jozkee
Copy link
Member

@jozkee jozkee commented Jul 30, 2019

APIs documented:

  • ASCIIEncoding
    • GetByteCount(System.ReadOnlySpan{System.Char})
    • GetBytes(System.ReadOnlySpan{System.Char},System.Span{System.Byte})
    • GetCharCount(System.ReadOnlySpan{System.Byte})
    • GetChars(System.ReadOnlySpan{System.Byte},System.Span{System.Char})
  • UnicodeEncoding.Preamble
  • UTF32Encoding.Preamble
  • UTF8Encoding
    • GetByteCount(System.ReadOnlySpan{System.Char})
    • GetBytes(System.ReadOnlySpan{System.Char},System.Span{System.Byte})
    • GetCharCount(System.ReadOnlySpan{System.Byte})
    • GetChars(System.ReadOnlySpan{System.Byte},System.Span{System.Char})
    • Preamble

@jozkee
Copy link
Member Author

jozkee commented Jul 30, 2019

@mairaw @rpetrusha @carlossanlop

@carlossanlop carlossanlop added 🏁 Release: .NET Core 3.0 :checkered_flag: Release: .NET Core 3.0 new-content Indicates PRs that contain new articles waiting-on-reviews Indicates PRs that cannot be merged because of the lack of reviews labels Jul 30, 2019
@carlossanlop carlossanlop added this to the July 2019 milestone Jul 30, 2019
@mairaw mairaw modified the milestones: July 2019, August 2019 Jul 30, 2019
carlossanlop
carlossanlop reviewed Jul 30, 2019
View reviewed changes
xml/System.Text/UTF32Encoding.xml Outdated
## Remarks
The <xref:System.Text.UTF32Encoding> object can provide a preamble, which is an array of bytes that can be prefixed to the sequence of bytes resulting from the encoding process. Prefacing a sequence of encoded bytes with a byte order mark (code points U+0000 U+FEFF) helps the decoder determine the byte order and the transformation format, or UTF. The Unicode byte order mark (BOM) is serialized as follows (in hexadecimal):

- Big endian byte order: 00 00 FE FF
Copy link
Contributor

Choose a reason for hiding this comment

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

This would probably look better with:

Suggested change
- Big endian byte order: 00 00 FE FF
- Big endian byte order: `00 00 FE FF`

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok then I'll do the same on the Preamble property of the other classes.

carlossanlop
carlossanlop reviewed Jul 30, 2019
View reviewed changes
xml/System.Text/UTF32Encoding.xml Outdated

- Big endian byte order: 00 00 FE FF

- Little endian byte order: FF FE 00 00
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Little endian byte order: FF FE 00 00
- Little endian byte order: `FF FE 00 00`

tarekgh
tarekgh approved these changes Jul 30, 2019
View reviewed changes
Copy link
Member

@tarekgh tarekgh left a comment

Choose a reason for hiding this comment

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

LGTM,

Just one thing you may consider changing is, as we are dealing with the API's using Spans, would be nice in the text not to use phrases like "byte array" or "array size" and instead may use Span/Memory?

@jozkee
Copy link
Member Author

jozkee commented Jul 30, 2019

LGTM,

Just one thing you may consider changing is, as we are dealing with the API's using Spans, would be nice in the text not to use phrases like "byte array" or "array size" and instead may use Span/Memory?

@tarekgh good point, I will remove the word "array" from the Remarks so it can be inferred that we are talking about the span.

rpetrusha
rpetrusha suggested changes Jul 31, 2019
View reviewed changes
Copy link

@rpetrusha rpetrusha left a comment

Choose a reason for hiding this comment

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

This looks very good, @jozkee. I've left a number of suggestions for you to consider.

@rpetrusha rpetrusha removed the waiting-on-reviews Indicates PRs that cannot be merged because of the lack of reviews label Jul 31, 2019
@jozkee
Apply suggestions from code review
6db5eb9 
Co-Authored-By: Ron Petrusha <ronpet@microsoft.com>
@rpetrusha
Copy link

Thanks for the additional changes, @jozkee. I'll merge this PR now.

@rpetrusha rpetrusha merged commit 5572453 into dotnet:master Aug 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tarekgh tarekgh tarekgh approved these changes

@mairaw mairaw Awaiting requested review from mairaw

@GrabYourPitchforks GrabYourPitchforks Awaiting requested review from GrabYourPitchforks

@layomia layomia Awaiting requested review from layomia

@krwq krwq Awaiting requested review from krwq

+2 more reviewers

@carlossanlop carlossanlop carlossanlop left review comments

@rpetrusha rpetrusha rpetrusha approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@jozkee jozkee

Labels
🏁 Release: .NET Core 3.0 :checkered_flag: Release: .NET Core 3.0 new-content Indicates PRs that contain new articles
Projects
None yet
Milestone
August 2019
Development

Successfully merging this pull request may close these issues.
5 participants
@jozkee @rpetrusha @carlossanlop @tarekgh @mairaw