Skip to content

feat: Add StringSyntaxAttribute for guid and timespan args #48662

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

Closed
wants to merge 5 commits into from

Conversation

abc516
Copy link
Contributor

@abc516 abc516 commented Jun 7, 2023

#44535 (GuidFormat and TimeSpanFormat)

  • You've read the Contributor Guide and Code of Conduct.
  • 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 the StringSyntaxAttribute for guid and timespan string function parameters, to specify syntax adherence.
Specifically, for the guid params, it adds it to all references of the SignalR Hub ConnectionId param and Kestrel Web Server ConnectionID .

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jun 7, 2023
@ghost
Copy link

ghost commented Jun 7, 2023

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

@ghost ghost added the area-signalr Includes: SignalR clients and servers label Jun 7, 2023
@BrennanConroy
Copy link
Member

I don't think the GuidFormat usages are correct. There is no restriction on ConnectionID being a GUID string. See https://source.dot.net/#Microsoft.AspNetCore.Http.Connections/Internal/HttpConnectionManager.cs,c33ed873441a4544 for an example of an ID we create that is explicitly not a GUID.

@abc516
Copy link
Contributor Author

abc516 commented Jun 7, 2023

@BrennanConroy I can remove them from the Http/Kestrel Server references. Do you have any recommendations to where I can put the GuidFormat usages?

@BrennanConroy
Copy link
Member

Do you have any recommendations to where I can put them GuidFormat usages?

There might not be any.

@ghost
Copy link

ghost commented Jun 19, 2023

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no breaking changes are introduced, please leave an /azp run comment here to rerun the CI pipeline and confirm success before merging the change.

@ghost ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jun 19, 2023
@davidfowl
Copy link
Member

I understand why this could be useful for the developers in this code base, but it doesn't seem like there's a big value in having these throughout internal and private methods in the code base.

@danmoseley
Copy link
Member

Based on the notes above, and this has sat for a while, I'm going to close. Thanks @abc516 for the contribution.

@danmoseley danmoseley closed this Sep 15, 2023
@ghost
Copy link

ghost commented Sep 15, 2023

Hi @danmoseley. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-signalr Includes: SignalR clients and servers community-contribution Indicates that the PR has been added by a community member pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants