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

Offer add parameter to constructor refactoring for all applicable constructors #34041

Merged
merged 14 commits into from
Mar 26, 2019

Conversation

chborl
Copy link
Contributor

@chborl chborl commented Mar 12, 2019

Fixes #33603
Fixes #33623

When one or more members are selected, user can Ctrl+. to add them as parameters to any applicable parameter. If more that one constructor is applicable, suggested actions will be nested under 2 menu items, "Add parameter to constructor" and "Add optional parameter to constructor"

In addition, this PR ensures parameters will not be offered for deserialization constructors.

image

@chborl chborl requested a review from a team as a code owner March 12, 2019 17:34
@chborl chborl changed the title Offer code refactorings for all applicable constructors Offer add parameter refactoring for all applicable constructors Mar 12, 2019
@CyrusNajmabadi
Copy link
Member

@chborl can you update your title to be clearer? Which feature is this for, for example.

@chborl chborl changed the title Offer add parameter refactoring for all applicable constructors Offer add parameter to constructor refactoring for all applicable constructors Mar 13, 2019
@chborl
Copy link
Contributor Author

chborl commented Mar 14, 2019

@CyrusNajmabadi, can you take another look? Thanks :)

JoeRobich and others added 4 commits March 14, 2019 14:53
Co-Authored-By: chborl <chborl@users.noreply.github.com>
Co-Authored-By: chborl <chborl@users.noreply.github.com>
sharwell and others added 2 commits March 15, 2019 15:43
Co-Authored-By: chborl <chborl@users.noreply.github.com>
Co-Authored-By: chborl <chborl@users.noreply.github.com>
@CyrusNajmabadi
Copy link
Member

stopping review. this looks like this still needs a pass by @chborl first to deal with the existing feedback.

@chborl chborl requested a review from sharwell March 21, 2019 00:11
@chborl
Copy link
Contributor Author

chborl commented Mar 21, 2019

@sharwell and @CyrusNajmabadi for review. Thanks :)

sharwell and others added 3 commits March 21, 2019 10:33
Co-Authored-By: chborl <chborl@users.noreply.github.com>
Co-Authored-By: chborl <chborl@users.noreply.github.com>
Update doc comment

Co-Authored-By: chborl <chborl@users.noreply.github.com>
@dpoeschl
Copy link
Contributor

Apologies if I missed an existing conversation on this, but should we need to limit 1) The number of constructors listed in these submenus, or 2) The length of the constructor symbol display?

@chborl
Copy link
Contributor Author

chborl commented Mar 25, 2019

Apologies if I missed an existing conversation on this, but should we need to limit 1) The number of constructors listed in these submenus, or 2) The length of the constructor symbol display?

@dpoeschl , here is a screenshot with 16 constructors:
image

The width of the sub menu is limited with .... As for the length of the sub menu, since these are shown in a submenu they won't push other refactorings further down so I don't think it's a problem. We can always address it in a follow-up issue if people report issues with it.

Copy link
Contributor

@dpoeschl dpoeschl left a comment

Choose a reason for hiding this comment

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

👍

@chborl chborl merged commit a515307 into dotnet:master Mar 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants