Skip to content

Refactoring to add DebuggerDisplay #38090

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 31 commits into from
Mar 22, 2020
Merged

Refactoring to add DebuggerDisplay #38090

merged 31 commits into from
Mar 22, 2020

Conversation

@jnm2
Copy link
Contributor

@jnm2 jnm2 commented Aug 19, 2019 •
edited
Loading

Closes #37990

@jnm2 jnm2 requested a review from a team as a code owner August 19, 2019 02:48
@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Aug 19, 2019

LMK if/when you want review.

CyrusNajmabadi
CyrusNajmabadi reviewed Aug 19, 2019
View reviewed changes
@sharwell sharwell added Area-IDE Blocked Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Aug 19, 2019
@sharwell
Copy link
Contributor

sharwell commented Aug 19, 2019

Blocked on the design review in #37990

@jnm2 jnm2 force-pushed the debuggerdisplay branch 2 times, most recently from a68008d to cf6e310 Compare August 19, 2019 23:55
@jnm2 jnm2 changed the title Refactoring to add [DebuggerDisplay("{ToString(),nq}")] Refactoring to add [DebuggerDisplay("{GetDebuggerDisplay(),nq}")] Sep 12, 2019
@jnm2 jnm2 force-pushed the debuggerdisplay branch 2 times, most recently from ce111d7 to b027a8e Compare September 13, 2019 02:41
@sharwell sharwell removed the Blocked label Sep 13, 2019
@jnm2 jnm2 force-pushed the debuggerdisplay branch 2 times, most recently from 49e0a53 to 2894067 Compare September 13, 2019 03:37
@jnm2 jnm2 requested review from CyrusNajmabadi and removed request for a team September 13, 2019 03:37
@jnm2
Copy link
Contributor Author

jnm2 commented Sep 13, 2019

Resolved review comments, updated to match the design spec. Ready for review!

@sharwell I broke something... all I did was click the refresh icon next to Cyrus's review:

image

sharwell
sharwell reviewed Sep 13, 2019
View reviewed changes
src/EditorFeatures/CSharpTest/AddDebuggerDisplay/AddDebuggerDisplayTests.cs Outdated
Copy link
Contributor

@sharwell sharwell Sep 13, 2019

Choose a reason for hiding this comment

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

💭 This doesn't seem like an ideal place to add it.

Copy link
Contributor Author

@jnm2 jnm2 Sep 13, 2019

Choose a reason for hiding this comment

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

My thinking was that this normally occurs to me right after I implement ToString, but I'm happy to remove it.

Copy link
Contributor

@sharwell sharwell Sep 13, 2019

Choose a reason for hiding this comment

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

On further thought, the three places I could see this relevant are:

  1. The type name token (low priority)
  2. The signature of ToString() (low priority)
  3. The signature of GetDebuggerDisplay() (normal priority, or potentially even with a diagnostic analyzer)

Copy link
Contributor Author

@jnm2 jnm2 Sep 14, 2019

Choose a reason for hiding this comment

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

I can do that. What's the rationale on having it available on the type name token but not the beginning of the line?

Copy link
Contributor Author

@jnm2 jnm2 Sep 17, 2019

Choose a reason for hiding this comment

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

@sharwell I think I'd like to be able to invoke this at the beginning of the line. What do you think about that?

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Mar 10, 2020

Choose a reason for hiding this comment

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

@jnm2 did you implement this?

@sharwell
Copy link
Contributor

sharwell commented Feb 21, 2020

📝 I verified commit e16026b is accurate with respect to the originally-reviewed code.

@jnm2
Copy link
Contributor Author

jnm2 commented Feb 21, 2020

(Thanks btw, I'll merge next time!)

@jnm2
Copy link
Contributor Author

jnm2 commented Mar 10, 2020

Fixed merge conflicts with a merge commit.

It has been five months since there was any feedback on the PR. There are two approvals. Is there anything I can do to clear the way for this to be merged? This is a feature that I miss on a weekly basis.

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Mar 10, 2020

I'll buddy this.

@CyrusNajmabadi CyrusNajmabadi self-assigned this Mar 10, 2020
@CyrusNajmabadi CyrusNajmabadi changed the base branch from master to release/dev16.7-preview1 March 21, 2020 22:23
@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Mar 22, 2020

Thanks!

@CyrusNajmabadi CyrusNajmabadi merged commit beb2cb1 into dotnet:release/dev16.7-preview1 Mar 22, 2020
@CyrusNajmabadi CyrusNajmabadi mentioned this pull request Mar 22, 2020
Closed
@jinujoseph jinujoseph added this to the 16.7.P1 milestone Mar 22, 2020
@jnm2 jnm2 deleted the debuggerdisplay branch March 22, 2020 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi approved these changes

+4 more reviewers

@petrroll petrroll petrroll left review comments

@mavasani mavasani mavasani left review comments

@YairHalberstadt YairHalberstadt YairHalberstadt left review comments

@sharwell sharwell sharwell approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@sharwell sharwell

@CyrusNajmabadi CyrusNajmabadi

Labels

Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. Feature Request

Projects

None yet

Milestone

16.7.P1

Development

Successfully merging this pull request may close these issues.

Refactoring to add [DebuggerDisplay("{GetDebuggerDisplay(),nq}")]

9 participants

@jnm2 @CyrusNajmabadi @sharwell @YairHalberstadt @RikkiGibson @mavasani @tmat @petrroll @jinujoseph