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

Spade: Option to Display Type Information, #263, disabled by default #582

Merged
merged 6 commits into from Oct 21, 2018

Conversation

Projects
None yet
2 participants
@GammaWolf
Copy link
Contributor

GammaWolf commented Aug 17, 2018

No description provided.

@codecadwallader

This comment has been minimized.

Copy link
Owner

codecadwallader commented Aug 18, 2018

Thanks for the PR. Overall it looks good. :) Two suggestions:

  1. Instead of calling it "Show Types" how about "Show Return Types" since it is just for controlling the visualization of return types? We already show types for arguments always.
  2. We use a different display style for argument types.. should we considering using the same for return types?
@GammaWolf

This comment has been minimized.

Copy link
Contributor

GammaWolf commented Aug 19, 2018

I have made the changes.

  1. I had also noticed the naming issue but wasn't sure to include 'return' since it also controls the visualization of fields and events which I wouldn't see as having return types. But it's probably better having it than having the conflict with it not controlling the types in the arguments.
@codecadwallader

This comment has been minimized.

Copy link
Owner

codecadwallader commented Aug 25, 2018

  1. Yeah that's a good point. The underlying EnvDTE generally just calls it the "type" of the item. Looking at the other options perhaps "Show item type" would be more accurate? Thoughts?

I noticed that in the converter we're directly reading from settings, but before the converter was settings agnostic and instead had properties/overloaded default instances for the different settings choices. I think either way works, but I'd like to have it be the same so there's not two different ways one converter is getting settings state. I can make these changes if that isn't clear or if you want to that would be great.

@GammaWolf

This comment has been minimized.

Copy link
Contributor

GammaWolf commented Sep 3, 2018

  1. I don't think it makes much of a difference. I'd say just pick one.

I didn't think the overhead of passing it in was worth it in this case. Also, I didn't see a good way to do it, since it's done with triggers in CodeItemTemplates.xaml, which seems like it doesn't fit well with combination of settings. Feel free to wire it up as you like.

@codecadwallader

This comment has been minimized.

Copy link
Owner

codecadwallader commented Sep 16, 2018

I've simplified the XAML and have the converter reading from settings. I also changed the setting name to "Show item types" and enabled it by default. Do you want to take a look back over it and let me know if you see any other changes needed before we merge?

@GammaWolf

This comment has been minimized.

Copy link
Contributor

GammaWolf commented Oct 5, 2018

Looks good to me.

@codecadwallader codecadwallader merged commit 61a5d82 into codecadwallader:master Oct 21, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@codecadwallader codecadwallader modified the milestones: v11.0, v10.6 Oct 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment