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

Revert "Add params ReadOnlySpan<T> overloads" #101123

Merged
merged 1 commit into from
Apr 17, 2024

Conversation

jozkee
Copy link
Member

@jozkee jozkee commented Apr 16, 2024

@jozkee jozkee requested a review from stephentoub April 16, 2024 15:40
@dotnet-issue-labeler dotnet-issue-labeler bot added needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners new-api-needs-documentation labels Apr 16, 2024
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, 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.

@jozkee jozkee added area-System.Memory and removed new-api-needs-documentation needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Apr 16, 2024
@jozkee jozkee added this to the 9.0.0 milestone Apr 16, 2024
Copy link
Contributor

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

@akoeplinger
Copy link
Member

/ba-g the failure in Build Analysis is a false-positive, the console log shows the test passed but there was some infrastructure error when uploading results to AzDO.

@akoeplinger akoeplinger merged commit 3e569f5 into main Apr 17, 2024
144 of 150 checks passed
@akoeplinger akoeplinger deleted the revert-100898-paramsspan branch April 17, 2024 10:28
@stephentoub
Copy link
Member

@jozkee, all the known issues were due to params, right? Could we resubmit this to add all the tests and all the new overloads, just without params on them? Once the language issues are addressed, we could then subsequently follow up to just add the keyword.

@jozkee
Copy link
Member Author

jozkee commented Apr 17, 2024

As per dotnet/aspnetcore#55010 (comment), string.Join(string?, params ReadOnlySpan<string?>) would cause a source breaking change. What's our policy on that kind of changes?

cc @bartonjs.

@bartonjs
Copy link
Member

From a guidelines perspective, StringValues shouldn't have had the implicit conversion operators, and then there wouldn't have been a problem :).

My guess is that we'll decide that this compile break is the right thing for the platform and resubmit; and anyone passing a StringValues as the only argument to Join or Format or what have you will just have to write in a cast to show which thing they meant... but that sounds like something we should decorate for API Review, mark it blocking, and discuss on Tuesday.

@stephentoub
Copy link
Member

I agree with @bartonjs. However, even if it were an issue, that particular case is still only an issue because of the params, right? So for now we can still put back the new overloads, just without params, as I'd suggested?

@jozkee
Copy link
Member Author

jozkee commented Apr 19, 2024

@stephentoub right, I just sent #101308 for that.

jozkee added a commit to jozkee/runtime that referenced this pull request Apr 19, 2024
jozkee added a commit that referenced this pull request Apr 23, 2024
* Reapply "Add params ReadOnlySpan<T> overloads (#100898)" (#101123)

This reverts commit 3e569f5.

* Comment-out params keyword

* Remove /*params*/ from ref
matouskozak pushed a commit to matouskozak/runtime that referenced this pull request Apr 30, 2024
matouskozak pushed a commit to matouskozak/runtime that referenced this pull request Apr 30, 2024
…101308)

* Reapply "Add params ReadOnlySpan<T> overloads (dotnet#100898)" (dotnet#101123)

This reverts commit 3e569f5.

* Comment-out params keyword

* Remove /*params*/ from ref
michaelgsharp pushed a commit to michaelgsharp/runtime that referenced this pull request May 9, 2024
…101308)

* Reapply "Add params ReadOnlySpan<T> overloads (dotnet#100898)" (dotnet#101123)

This reverts commit 3e569f5.

* Comment-out params keyword

* Remove /*params*/ from ref
@github-actions github-actions bot locked and limited conversation to collaborators May 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants