Skip to content

Add Uri StringSyntaxAttribute syntaxes#44570

Merged
JamesNK merged 6 commits intodotnet:mainfrom
blouflashdb:addUriStringSyntaxAttribute
Oct 17, 2022
Merged

Add Uri StringSyntaxAttribute syntaxes#44570
JamesNK merged 6 commits intodotnet:mainfrom
blouflashdb:addUriStringSyntaxAttribute

Conversation

@blouflashdb
Copy link
Contributor

@blouflashdb blouflashdb commented Oct 15, 2022

Add Uri StringSyntaxAttribute syntaxes

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Description

Adds uri StringSyntaxAttribute syntaxes to all public APIs where an uri is expected.

Closes Uri Task in #44535

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Oct 15, 2022
@ghost
Copy link

ghost commented Oct 15, 2022

Thanks for your PR, @blouflashdb. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@blouflashdb
Copy link
Contributor Author

blouflashdb commented Oct 15, 2022

Well somehow I can't use it in HubConnectionBuilderHttpExtensions.cs for some reason.
Check failure on line 26 in src/SignalR/clients/csharp/Client/src/HubConnectionBuilderHttpExtensions.cs

Azure Pipelines
/ aspnetcore-ci (Build Build: Linux ARM)
src/SignalR/clients/csharp/Client/src/HubConnectionBuilderHttpExtensions.cs#L26
src/SignalR/clients/csharp/Client/src/HubConnectionBuilderHttpExtensions.cs(26,131): error CS0122: (NETCORE_ENGINEERING_TELEMETRY=Build) 'StringSyntaxAttribute' is inaccessible due to its protection level

@JamesNK
Copy link
Member

JamesNK commented Oct 16, 2022 •
edited
Loading

The SignalR client also targets .NET Framework, which doesn't have that attribute.

To work around this:

  1. Copy the attribute's source code from StringSyntaxAttribute.cs
  2. Add an internal polyfill type in the Shared/CodeAnalysis directory - https://github.com/dotnet/aspnetcore/tree/1b53b22a866eb4bc58f2d5552db7f3ac08f1a952/src/Shared/CodeAnalysis. The trimming types already in that directory are a good example of this technique.
  3. Finally, add a link to the shared source file from the SignalR client project.

It doesn't matter that the attribute is internal and in a different assembly. As long as the type name and namespace match, tooling and other apps can use it.

@blouflashdb
Copy link
Contributor Author

blouflashdb commented Oct 16, 2022

Thanks, I think I got it.

JamesNK
JamesNK reviewed Oct 17, 2022
View reviewed changes
JamesNK
JamesNK reviewed Oct 17, 2022
View reviewed changes
Copy link
Member

@JamesNK JamesNK left a comment

Choose a reason for hiding this comment

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

RelativeOrAbsolute would be the default assumption so there is no need to explicitly specify it.

Otherwise, LGTM

@JamesNK JamesNK merged commit fa2fd21 into dotnet:main Oct 17, 2022
@ghost ghost added this to the 8.0-preview1 milestone Oct 17, 2022
@blouflashdb blouflashdb deleted the addUriStringSyntaxAttribute branch October 17, 2022 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JamesNK JamesNK JamesNK approved these changes

@BrennanConroy BrennanConroy Awaiting requested review from BrennanConroy BrennanConroy is a code owner

@halter73 halter73 Awaiting requested review from halter73 halter73 is a code owner

@Tratcher Tratcher Awaiting requested review from Tratcher

Assignees

No one assigned

Labels

community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Milestone

8.0-preview1

Development

Successfully merging this pull request may close these issues.

2 participants

@blouflashdb @JamesNK