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.

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

First try at implementing newly suggested ctor for providing tighter control over charset #63231

Merged
merged 3 commits into from
Mar 9, 2022
Merged

First try at implementing newly suggested ctor for providing tighter control over charset #63231

merged 3 commits into from
Mar 9, 2022

Conversation

brianmed
Copy link
Contributor

@brianmed brianmed commented Dec 31, 2021 •
edited by karelz
Loading

There are three new ctor implementations.

All are from issue #17036.

public StringContent(string content, MediaTypeHeaderValue mediaType)
public StringContent(string content, Encoding encoding, MediaTypeHeaderValue mediaType)
public MediaTypeHeaderValue(string mediaType, string charSet)

Also, there are tests for the new APIs.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Dec 31, 2021
@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Dec 31, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

There are three new ctor implementations.

All are from issue #17036.

public StringContent(string content, MediaTypeHeaderValue mediaType)
public StringContent(string content, Encoding encoding, MediaTypeHeaderValue mediaType)
public MediaTypeHeaderValue(string mediaType, string charSet)

Also, there are tests for the new APIs.

Author: brianmed
Assignees: -
Labels:

area-System.Net.Http, new-api-needs-documentation, community-contribution
Milestone: -

@dnfadmin
Copy link

dnfadmin commented Dec 31, 2021 •
edited
Loading

CLA assistant check
All CLA requirements met.

@scalablecory scalablecory self-requested a review December 31, 2021 03:48
AraHaan
AraHaan reviewed Dec 31, 2021
View reviewed changes
src/libraries/System.Net.Http/ref/System.Net.Http.cs
@@ -437,8 +437,10 @@ public partial class StreamContent : System.Net.Http.HttpContent
public partial class StringContent : System.Net.Http.ByteArrayContent
{
public StringContent(string content) : base (default(byte[])) { }
public StringContent(string content, System.Net.Http.Headers.MediaTypeHeaderValue mediaType) : base (default(byte[])) { }
Copy link
Member

@AraHaan AraHaan Dec 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I thought that for reference code that base() should never be used. Of course this was what I thought when looking and changing the ones in System.IO.Compression.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AraHaan I'm actually clueless on that point. What should be used?

Copy link
Member

Choose a reason for hiding this comment

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

Well, I see that it already uses base() so that file should be fine to do it in.

Copy link
Member

Choose a reason for hiding this comment

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

There is a markdown doc in this repo explaining how to update the ref files automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@danmoseley Thank you for letting me know. After I follow the instructions in updating-ref-source.md I'm getting a large number of changes. Many of those changes I did not do.

Copy link
Member

Choose a reason for hiding this comment

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

The ideal, I think, is that the ref files are completely and only generated output. cc @safern for whether that's correct.

Meantime I suggest to just include relevant changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just did a commit that hopefully updated the reference assembly correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@danmoseley Was my latest commit sufficient?

Copy link
Member

Choose a reason for hiding this comment

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

@brianmed looks right as far as I can tell, thanks. I guess @scalablecory plans to review now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@danmoseley Just updated. Sorry it took a few days.

@brianmed brianmed requested a review from AraHaan December 31, 2021 05:30
@runfoapp runfoapp bot mentioned this pull request Jan 3, 2022
Closed
@scalablecory
Copy link
Contributor

PR looks good to me, only nits to address. I believe @brianmed is away for a week or two without internet.

@karelz
Copy link
Member

karelz commented Feb 1, 2022

Hello @brianmed, are you back from vacation? Do you have rough ETA when you think you can get to the remaining feedback? Thanks!

scalablecory
scalablecory approved these changes Feb 8, 2022
View reviewed changes
Copy link
Contributor

@scalablecory scalablecory left a comment

Choose a reason for hiding this comment

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

lgtm.

stephentoub
stephentoub approved these changes Feb 8, 2022
View reviewed changes
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

thanks!

@karelz karelz added this to the 7.0.0 milestone Feb 22, 2022
@karelz karelz self-assigned this Feb 22, 2022
@ManickaP ManickaP assigned ManickaP and unassigned karelz Mar 8, 2022
@ManickaP ManickaP merged commit f132942 into dotnet:main Mar 9, 2022
@stephentoub stephentoub mentioned this pull request Mar 16, 2022
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Apr 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@danmoseley danmoseley danmoseley left review comments

@stephentoub stephentoub stephentoub approved these changes

@scalablecory scalablecory scalablecory approved these changes

@AraHaan AraHaan Awaiting requested review from AraHaan

Assignees

@ManickaP ManickaP

Labels
area-System.Net.Http community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.
8 participants
@brianmed @dnfadmin @scalablecory @karelz @stephentoub @danmoseley @AraHaan @ManickaP