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

Change option version #16373

Merged

Conversation

jmarolf
Copy link
Contributor

@jmarolf jmarolf commented Jan 10, 2017

Customer scenario

Customer changes setting on a VS 2017 Machine and then their settings on their VS 2015 machine stop syncing and they receive an error message
Customer changes setting on a VS 2015 Machine and then their settings on their VS 2017 machine stop syncing and they receive an error message

Bugs this fixes:

#13556
#15076
#273302

Workarounds, if any

Do not roam any settings between VS 2015 or VS 2017. If you roam settings on VS 2015 do not roam them on VS 2017.

Risk
Low, we are changing a key name.

Performance impact

None, we are changing a key name.

Is this a regression from a previous update?
Yes, you could previously roam settings between versions

Root cause analysis:

in Dev14 these options were persisted as booleans. They are now persisted as xml in Dev15. The fix is to not have the persistence keys overlap, ensuring Dev14 will not try to deserialize Dev15 options and vice versa

How was the bug found?

Customer reported and found via ad-hoc testing

@Pilchie @jasonmalinowski @dotnet/roslyn-ide

@jmarolf jmarolf added this to the 2.0 (RTM) milestone Jan 10, 2017
@@ -55,15 +55,15 @@ public class CodeStyleOptions
public static readonly PerLanguageOption<CodeStyleOption<bool>> PreferIntrinsicPredefinedTypeKeywordInDeclaration = new PerLanguageOption<CodeStyleOption<bool>>(nameof(CodeStyleOptions), nameof(PreferIntrinsicPredefinedTypeKeywordInDeclaration), defaultValue: TrueWithNoneEnforcement,
storageLocations: new OptionStorageLocation[]{
new EditorConfigStorageLocation("dotnet_style_predefined_type_for_locals_parameters_members"),
new RoamingProfileStorageLocation("TextEditor.%LANGUAGE%.Specific.PreferIntrinsicPredefinedTypeKeywordInDeclaration")});
new RoamingProfileStorageLocation("TextEditor.%LANGUAGE%.Specific.PreferIntrinsicPredefinedTypeKeywordInDeclaration.Dev15")});
Copy link
Member

Choose a reason for hiding this comment

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

Given this would be used for any mythical post-Dev15 thing, perhaps call it it .CodeStyle or something as a more agnostic suffix?

(I'm getting visions of Common7 all over again.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm happy to change it. I chose Dev15 because this option will only be persisted to machines with Dev15 or newer. Code Style is fine though.

@Pilchie
Copy link
Member

Pilchie commented Jan 10, 2017

How does this work with "Reset Settings", and the shipping .vssettings files? They go through the automation object - will they still be applied properly?

Copy link
Member

@Pilchie Pilchie left a comment

Choose a reason for hiding this comment

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

This change looks fine, but I want to make sure we understand the relationship with .vssettings files too.

@Pilchie
Copy link
Member

Pilchie commented Jan 10, 2017

Should target dev15-rc3.

@Pilchie Pilchie modified the milestones: 2.0 (RC.3), 2.0 (RTM) Jan 10, 2017
@jmarolf jmarolf changed the base branch from master to dev15-rc3 January 10, 2017 01:07
in Dev14 these options were persisted as booleans. They are now persisted as xml in Dev15.  The fix is to not have the persistence keys overlap, ensuring Dev14 will not try to deserialize Dev15 options and vice versa
@jmarolf
Copy link
Contributor Author

jmarolf commented Jan 10, 2017

@Pilchie .vssettings files will need to be updated if we want reset to work. If you Reset Settings in VS 2017 it does not change any of the code style settings. With this change the default settings Style_PreferIntrinsicPredefinedTypeKeywordInMemberAccess and Style_PreferIntrinsicPredefinedTypeKeywordInDeclaration will be ignored on reset, but they can be exported and then imported.

@jmarolf
Copy link
Contributor Author

jmarolf commented Jan 10, 2017

retest linux_debug_prtest please

@Pilchie
Copy link
Member

Pilchie commented Jan 10, 2017

Okay - we've got another bug tracking the fact that some of our options don't import/export properly. Let's take this one now, and deal with .vssettings files as part of #16392/#16393.

@Pilchie
Copy link
Member

Pilchie commented Jan 10, 2017

@jasonmalinowski care to take another look? @jmarolf made the suggested rename.

@MattGertz for shiproom consideration.

@jasonmalinowski
Copy link
Member

@jmarolf, just so I understand, we're renaming the .vssettings file since the underlying format changed (Boolean to codestyle) that even if we didn't rename it they wouldn't practically apply from one to the other?

Since there are extra type guards I don't think the .vssettings change is necessary. And if we care about applying old team settings forward, actually hinders fixing that. But I'm not sure it's important either way.

@natidea
Copy link
Contributor

natidea commented Jan 10, 2017

Approved to merge

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

Successfully merging this pull request may close these issues.

None yet

6 participants