Skip to content

Allow @ as separator in #:package #48542

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

Merged
merged 1 commit into from
Apr 18, 2025
Merged

Allow @ as separator in #:package #48542

merged 1 commit into from
Apr 18, 2025

Conversation

jjonescz
Copy link
Member

Resolves #47990.

@jjonescz jjonescz added the Area-run-file Items related to the "dotnet run <file>" effort label Apr 17, 2025
@jjonescz jjonescz requested a review from a team April 17, 2025 11:45
@ghost ghost added the untriaged Request triage from a team member label Apr 17, 2025
@jjonescz jjonescz marked this pull request as ready for review April 17, 2025 13:03
@Copilot Copilot AI review requested due to automatic review settings April 17, 2025 13:03
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

src/Cli/dotnet/Commands/Run/VirtualProjectBuildingCommand.cs:820

  • [nitpick] Consider renaming the local variable 'i' to a more descriptive name (e.g., 'separatorIndex') to enhance code clarity.
var i = separators != null ? directiveText.AsSpan().IndexOfAny(separators) : directiveText.IndexOf(' ', StringComparison.Ordinal);

Copy link
Member

@RikkiGibson RikkiGibson left a comment

Choose a reason for hiding this comment

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

So are we planning to be tolerant of different separators here for the short term? Or maybe indefinitely? And are there other changes coming for the allowed/permitted separators for #:sdk and #:property?

@jjonescz
Copy link
Member Author

jjonescz commented Apr 17, 2025

So are we planning to be tolerant of different separators here for the short term? Or maybe indefinitely? And are there other changes coming for the allowed/permitted separators for #:sdk and #:property?

Good questions.

If we are doing @ for packages, maybe we want also / for sdk to be consistent with <Project Sdk="Name/Version"> syntax.

We could also have a "breaking change" and say spaces are not allowed for packages (and sdk if we change that as well). I would actually prefer that as there would be just one common way to specify the same directives. (The downside might be you cannot choose one separator - space - for everything like you can today.) But curious what others think.

@RikkiGibson
Copy link
Member

I would prefer that only one separator is permitted for each directive kind. But, I also think being tolerant during the experimental phase is fine, to allow people to "offboard" from spaces as delimiters.

@jjonescz jjonescz enabled auto-merge (squash) April 18, 2025 19:22
@jjonescz jjonescz merged commit 600b00d into dotnet:main Apr 18, 2025
41 checks passed
@jjonescz jjonescz deleted the sprint-at branch April 18, 2025 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-run-file Items related to the "dotnet run <file>" effort untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Align #:package syntax with CLI for specifying package ID and version
5 participants