Skip to content

change 'ValuesController' to 'ValueController' #47474

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
Apr 25, 2023
Merged

change 'ValuesController' to 'ValueController' #47474

merged 1 commit into from
Apr 25, 2023

Conversation

deepchoudhery
Copy link
Member

@deepchoudhery deepchoudhery commented Mar 28, 2023 •
edited
Loading

  • changed references in Web.ItemTemplates\content\ApiController\ValueController.cs from ValuesController to ValueController
  • consistent with the all other references
  • fixed --name parameter

@ghost ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Mar 28, 2023
@deepchoudhery deepchoudhery self-assigned this Mar 28, 2023
@mitchdenny
Copy link
Member

Is this just a preference for singular over plural controller names?

@mitchdenny mitchdenny added the Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. label Mar 31, 2023
@ghost
Copy link

ghost commented Mar 31, 2023

Hello. I see that you've just added Needs: Author Feedback label to this PR.
That label is for Issues and not for PRs. Don't worry, I'm going to replace it with the correct one.

@ghost ghost added pr: pending author input For automation. Specifically separate from Needs: Author Feedback and removed Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. labels Mar 31, 2023
@deepchoudhery
Copy link
Member Author

Is this just a preference for singular over plural controller names?

It actually fixes the issue with --name param working correctly since the template.json and the actual file ValueController.cs were out of sync with the reference ValueController. Need them matching for the parameter working.

@ghost
Copy link

ghost commented Apr 12, 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 Apr 12, 2023
@ghost
Copy link

ghost commented Apr 22, 2023

Hi @deepchoudhery.
It seems you haven't touched this PR for the last two weeks. To avoid accumulating old PRs, we're marking it as stale. As a result, it will be closed if no further activity occurs within 4 days of this comment. You can learn more about our Issue Management Policies here.

@ghost ghost added the stale Indicates a stale issue. These issues will be closed automatically soon. label Apr 22, 2023
@mkArtakMSFT mkArtakMSFT removed pr: pending author input For automation. Specifically separate from Needs: Author Feedback stale Indicates a stale issue. These issues will be closed automatically soon. labels Apr 24, 2023
@mkArtakMSFT
Copy link
Contributor

Thanks @deepchoudhery. Can you share the customer impact please?
Specifically what was not working for the customer?

As a side note, I do agree that the class name should have matched the file name here, and it was most probably a mistake done when this was created.

@deepchoudhery
Copy link
Member Author

deepchoudhery commented Apr 25, 2023 •
edited
Loading

Thanks @deepchoudhery. Can you share the customer impact please? Specifically what was not working for the customer?

As a side note, I do agree that the class name should have matched the file name here, and it was most probably a mistake done when this was created.

Customer Impact : Customers who want to name their ApiController using the --name, controller will not be named correctly.

It was a mistake when we first committed this template to aspnetcore, yes.

mkArtakMSFT
mkArtakMSFT approved these changes Apr 25, 2023
View reviewed changes
Copy link
Contributor

@mkArtakMSFT mkArtakMSFT left a comment

Choose a reason for hiding this comment

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

Thanks @deepchoudhery !

@mkArtakMSFT
Copy link
Contributor

@deepchoudhery can you merge this, or do you want me to ?

@deepchoudhery
Copy link
Member Author

@deepchoudhery can you merge this, or do you want me to ?

I got it, tyty

@deepchoudhery deepchoudhery merged commit 47d0784 into main Apr 25, 2023
@deepchoudhery deepchoudhery deleted the dev/decho/fix-apicontroller-template branch April 25, 2023 18:53
@ghost ghost added this to the 8.0-preview4 milestone Apr 25, 2023
@mkArtakMSFT mkArtakMSFT removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkArtakMSFT mkArtakMSFT mkArtakMSFT approved these changes

@Nick-Stanton Nick-Stanton Awaiting requested review from Nick-Stanton

@mitchdenny mitchdenny Awaiting requested review from mitchdenny

Assignees

@deepchoudhery deepchoudhery

Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Milestone
8.0-preview4
Development

Successfully merging this pull request may close these issues.
3 participants
@deepchoudhery @mitchdenny @mkArtakMSFT