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

update project templates use primary constructors #59712

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Varorbc
Copy link
Contributor

@Varorbc Varorbc commented Jan 4, 2025

update project templates use primary constructors

  • 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

Fixes #52893

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Jan 4, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jan 4, 2025
@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 Jan 11, 2025
@Varorbc
Copy link
Contributor Author

Varorbc commented Jan 25, 2025

@danroth27 ping

@danmoseley
Copy link
Member

@Varorbc apologies for delay.
@captainsafia I believe you own area-mvc which includes most templates? could you have a look?

Copy link
Member

@captainsafia captainsafia left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution!

It looks great overall! The move to primary constructors here has revealed places where we were initializing but not using ILoggers. I think it might be worthwhile to continue to inject ILogger's into the classes via the primary constructor but add code to use them in the class so that the functionality is properly demonstrated.

@@ -1,16 +1,9 @@
using Grpc.Core;
using GrpcService_CSharp;

namespace GrpcService_CSharp.Services;

public class GreeterService : Greeter.GreeterBase
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Although the ILogger<GreeterService> was unused before perhaps we can add a log to the SayHello method to more fully demonstrate primary constructors here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I moved things to the main constructor, I noticed a bunch of log initializations that weren't being used, which caused our CI to fail. Since I wasn't sure what kind of logs should be output, I had no choice but to remove them for now. If you could provide the correct log outputs for each of the removed logs, that would be perfect.

@@ -9,7 +9,8 @@

var app = builder.Build();

var sampleTodos = new Todo[] {
var sampleTodos = new Todo[]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var sampleTodos = new Todo[]
Todo[] sampleTodos = [

Small nit unrelated to your contribution but while we're here perhaps we should update this to use collection expressions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates 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.

update project templates use primary constructors
3 participants