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

Add StringSyntax formats throughout source code #44535

Open
9 of 13 tasks
JamesNK opened this issue Oct 14, 2022 · 25 comments
Open
9 of 13 tasks

Add StringSyntax formats throughout source code #44535

JamesNK opened this issue Oct 14, 2022 · 25 comments
Assignees
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates good first issue Good for newcomers. help candidate Indicates that the issues may be a good fit for community to help with. Requires work from eng. team
Milestone

Comments

@JamesNK
Copy link
Member

JamesNK commented Oct 14, 2022

Is there an existing issue for this?

  • I have searched the existing issues

Is your feature request related to a problem? Please describe the problem.

StringSyntaxAttribute is placed on strings to tell tooling what format the string will take. For example, regex, JSON, XML, date format string, etc. This is then used to provide helpful features such as completion of date format strings, regex/JSON/XML highlighting, analyzers, etc.

We should ensure all the ASP.NET Core strings have this attribute where appropriate, so ASP.NET Core APIs get tooling enhancements.

Describe the solution you'd like

Audit dotnet/aspnetcore source and add StringSyntax attribute with format strings as needed:

  • CompositeFormat - The syntax identifier for strings containing composite formats for string formatting.
  • DateOnlyFormat - The syntax identifier for strings containing date format specifiers.
  • DateTimeFormat - The syntax identifier for strings containing date and time format specifiers.
  • EnumFormat - The syntax identifier for strings containing enum format specifiers.
  • GuidFormat - The syntax identifier for strings containing GUID format specifiers.
  • Json - The syntax identifier for strings containing JavaScript Object Notation (JSON).
  • NumericFormat - The syntax identifier for strings containing numeric format specifiers.
  • Regex - The syntax identifier for strings containing regular expressions. feat : Add StringSyntax for regex parameters #40589
  • TimeOnlyFormat - The syntax identifier for strings containing time format specifiers.
  • TimeSpanFormat - The syntax identifier for strings containing TimeSpan format specifiers.
  • Uri - The syntax identifier for strings containing URIs.
  • Xml - The syntax identifier for strings containing XML.

This list is from https://github.com/dotnet/runtime/blob/664d7c68d53f2465b79de25fdd6827007216239f/src/libraries/System.Private.CoreLib/src/System/Diagnostics/CodeAnalysis/StringSyntaxAttribute.cs#L38-L72

I expect many of these won't have any usage - e.g. the format strings (except CompositeFormat) - but we should have Regex, Uri, Json and Xml.

Additional context

No response

@danmoseley
Copy link
Member

Example of this kind of thing done previously in runtime
dotnet/runtime#67621

@danmoseley danmoseley added help wanted Up for grabs. We would accept a PR to help resolve this issue good first issue Good for newcomers. labels Oct 14, 2022
@blouflashdb
Copy link
Contributor

I would love to help here.

@danmoseley
Copy link
Member

I have assigned to you @blouflashdb

@blouflashdb
Copy link
Contributor

blouflashdb commented Oct 14, 2022

#44555 PR for CompositeFormat

more will follow soon.

@blouflashdb
Copy link
Contributor

@JamesNK I have a question about StringSyntaxAttribute.Uri.

There are some places where a RelativeIUri is passed as an parameter. I saw that optinal arguments can be added to StringSyntaxAttribute for example uriKind. How should the StringSyntaxAttribute look like for relative uri?

There is also one place in the code where a relative or absolute url can be passed to the methode. For Example WebViewManager.Navigate . How to handle this case?

@JamesNK
Copy link
Member Author

JamesNK commented Oct 15, 2022

Good question. Thanks for thinking about this.

I think StringSyntaxAttribute.Uri should be used for both relative and absolute URLs. For UriKind, an example of it is on the Uri ctor, and it looks like it the parameter named is passed as an argument on the attribute - https://github.com/dotnet/runtime/blob/9bcbf50d9ebe60cd83ed724179a5a503cbf03702/src/libraries/System.Private.Uri/src/System/Uri.cs#L405-L414

@stephentoub I see you added Uri throughout dotnet/runtime in dotnet/runtime#67621. Any extra input on these StringSyntaxAttribute usages?

@danmoseley
Copy link
Member

btw @CyrusNajmabadi just curious what the status of tooling support for these is? Do you expect them to all have an experience like regex has?

@blouflashdb
Copy link
Contributor

Does StringSyntaxAttribute also works on parameters like this? params string[] urls ?

@JamesNK
Copy link
Member Author

JamesNK commented Oct 15, 2022

I tested it, and yes it does:

image

However, looks like there is a bug in Roslyn where it only applies it to the first string. New Roslyn issue: dotnet/roslyn#64756

@CyrusNajmabadi
Copy link
Member

Do you expect them to all have an experience like regex has?

We can add support if the language provides a suitable parser we can depend on.

@CyrusNajmabadi
Copy link
Member

My preference though would be to treat it like asp route strings. Have the bcl team own the integration points, not Roslyn.

@mkArtakMSFT mkArtakMSFT added the area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Oct 19, 2022
@blouflashdb
Copy link
Contributor

There are no public api parameters where StringFormatAttribute Xml can be added.

@blouflashdb
Copy link
Contributor

There are some for DateOnlyFormat & DateTimeFormat. Others I haven't checked yet.

@ghost
Copy link

ghost commented Oct 20, 2022

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@stephentoub
Copy link
Member

@stephentoub I see you added Uri throughout dotnet/runtime in dotnet/runtime#67621. Any extra input on these StringSyntaxAttribute usages?

@JamesNK, what are you looking for feedback on here?

@JamesNK
Copy link
Member Author

JamesNK commented Oct 24, 2022

Original question was about arguments to URI strings. We figured it out.

@danmoseley
Copy link
Member

@blouflashdb is there more to do here, after your last changes? if so any interest in doing the last ones?

@blouflashdb
Copy link
Contributor

@danmoseley I will look into the remaining things aswell.

@danmoseley
Copy link
Member

Thanks @blouflashdb

@abc516
Copy link
Contributor

abc516 commented May 22, 2023

Hi @danmoseley @JamesNK, I notice this issue hasn't had activity for a while. Do you still need someone to take on the remaining tasks (e.g. GuidFormat)? Thanks.

@JamesNK
Copy link
Member Author

JamesNK commented May 23, 2023

It's already assigned but it has been idle for a while. @blouflashdb Do you plan to keep looking at this?

@blouflashdb
Copy link
Contributor

Sorry, I cant work on it anymore.

@danmoseley danmoseley assigned abc516 and unassigned blouflashdb May 23, 2023
@danmoseley
Copy link
Member

@blouflashdb thanks for the part you did. @abc516 thanks for taking on the rest!

@captainsafia captainsafia added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Jun 21, 2023
@cansozbir
Copy link

I wonder if NumericFormat is already done.

@mkArtakMSFT mkArtakMSFT added help candidate Indicates that the issues may be a good fit for community to help with. Requires work from eng. team and removed help wanted Up for grabs. We would accept a PR to help resolve this issue labels Oct 28, 2023
@ShirAvneri
Copy link

ShirAvneri commented Dec 1, 2023

@danmoseley I noticed that the last PR that addressed this issue was closed.
Would it be okay if I tried to work on this issue?

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
@captainsafia captainsafia modified the milestones: .NET 8 Planning, Backlog Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates good first issue Good for newcomers. help candidate Indicates that the issues may be a good fit for community to help with. Requires work from eng. team
Projects
No open projects
Status: No status
Development

No branches or pull requests