Skip to content

Show a message when building as part of dotnet-run #12581

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 23, 2020
Merged

Show a message when building as part of dotnet-run #12581

merged 1 commit into from
Jul 23, 2020

Conversation

@pranavkm
Copy link
Contributor

@pranavkm pranavkm commented Jul 22, 2020 •
edited
Loading

  • Tweak ILaunchSettingsProvider to return a LaunchSettingsResult
  • Read LaunchSettingsResult during dotnet-run to show message

Based on https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1154478

@pranavkm
Show a message when building as part of dotnet-run
7811050 
* Tweak ILaunchSettingsProvider to return a LaunchSettingsResult
* Read LaunchSettingsResult during dotnet-run to show message
@pranavkm pranavkm marked this pull request as ready for review July 22, 2020 01:09
wli3
wli3 suggested changes Jul 23, 2020
View reviewed changes
Copy link

@wli3 wli3 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

My forces is on RunCommand.cs. There are some code lacking coverage. And it is better to have unit tests to cover these code.

src/Cli/dotnet/commands/dotnet-run/RunCommand.cs
targetCommand.EnvironmentVariable("ASPNETCORE_URLS", launchSettings.ApplicationUrl);
}

foreach (var entry in launchSettings.EnvironmentVariables)
Copy link

@wli3 wli3 Jul 23, 2020

Choose a reason for hiding this comment

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

no test coverage for this code

src/Cli/dotnet/commands/dotnet-run/RunCommand.cs
return 1;
if (!string.IsNullOrEmpty(launchSettings.ApplicationUrl))
{
targetCommand.EnvironmentVariable("ASPNETCORE_URLS", launchSettings.ApplicationUrl);
Copy link

@wli3 wli3 Jul 23, 2020

Choose a reason for hiding this comment

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

no test coverage for this code

@pranavkm
Copy link
Contributor Author

pranavkm commented Jul 23, 2020

@wli3 I wrote a functional test for the scenarios you listed to make sure previously untested code worked after I basically moved the code around. While I understand it would be great to add unit test coverage for this type, this is not the PR to do it. I'll file an issue to add additional test coverage in this area.

@pranavkm pranavkm mentioned this pull request Jul 23, 2020
Closed
@pranavkm
Copy link
Contributor Author

pranavkm commented Jul 23, 2020

Filed #12609 as follow up.

@pranavkm pranavkm merged commit 285cac3 into release/5.0.1xx-preview8 Jul 23, 2020
@pranavkm pranavkm added this to the 5.0.1xx milestone Jul 23, 2020
@wli3
Copy link

wli3 commented Jul 23, 2020

While I understand it would be great to add unit test coverage for this type, this is not the PR to do it. I'll file an issue to add additional test coverage in this area.

I don't like this approach. And you dismissed my block directly. Unless it is really hard. Tests should go with the PR.

@marcpopMSFT marcpopMSFT mentioned this pull request Jul 29, 2020
Closed
@akoeplinger akoeplinger deleted the prkrishn/show-run-message branch December 17, 2020 19:50
warrenbuckley pushed a commit to umbraco/Umbraco-CMS that referenced this pull request Jun 15, 2021
Adds dotnetRunMessages flag to launch settings for Kestrel
ca1f750 
dotnet/sdk#12227
dotnet/sdk#12581
https://github.com/dotnet/aspnetcore/blob/v6.0.0-preview.5.21301.17/src/ProjectTemplates/Web.ProjectTemplates/content/StarterWeb-CSharp/Properties/launchSettings.json#L22
@warrenbuckley warrenbuckley mentioned this pull request Jun 15, 2021
Merged
bergmania pushed a commit to umbraco/Umbraco-CMS that referenced this pull request Jun 15, 2021
Adds dotnetRunMessages flag to launch settings for Kestrel (#10472)
ceb4db4 
dotnet/sdk#12227
dotnet/sdk#12581
https://github.com/dotnet/aspnetcore/blob/v6.0.0-preview.5.21301.17/src/ProjectTemplates/Web.ProjectTemplates/content/StarterWeb-CSharp/Properties/launchSettings.json#L22
@marcpopMSFT marcpopMSFT mentioned this pull request Jan 26, 2022
Closed
@KalleOlaviNiemitalo KalleOlaviNiemitalo mentioned this pull request Sep 10, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vijayrkn vijayrkn vijayrkn approved these changes

@DamianEdwards DamianEdwards Awaiting requested review from DamianEdwards

@BillHiebert BillHiebert Awaiting requested review from BillHiebert

+1 more reviewer

@wli3 wli3 wli3 requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

5.0.1xx

Development

Successfully merging this pull request may close these issues.

4 participants

@pranavkm @wli3 @vijayrkn