Clean up of EditorFeatures.Cocoa.Snippets #49188
Conversation
davidwengier
force-pushed
the
CocoaSnippets
branch
from
6ad2642 to
e9f083d
Compare
|
|
||
| // In Venus/Razor, inserting imports statements into the subject buffer does not work. | ||
| // Instead, we add the imports through the contained language host. | ||
| if (TryAddImportsToContainedDocument(document, newUsingDirectives.Where(u => u.Alias == null).Select(u => u.Name.ToString()))) |
There was a problem hiding this comment.
If you're not looking commit-by-commit this might look odd, but TryAddImportsToContainedDocument used to always throw, therefore the rest of this method was unreachable, therefore this whole AddImports method never did anything, which then meant the AddReferencesAndImports that called it also never did anything.
I'm guessing there are no snippets that add usings in VS for Mac??
There was a problem hiding this comment.
@Therzok @DavidKarlas @KirillOsenkov Does that make sense? Is this an extension point for VS for Mac? (not that I can see how it worked if it is)
There was a problem hiding this comment.
I'm guessing it was copied from here but we had no impl due to MonoDevelopWorkspace being in MonoDevelop, not platform assemblies:
There was a problem hiding this comment.
@davidwengier yes that make sense, no snippet inserts usings so I think its fine to delete this.
davidwengier
requested review from
KirillOsenkov,
DavidKarlas,
Therzok and
a team
| protected readonly IExpansionServiceProvider ExpansionServiceProvider; | ||
| protected readonly IExpansionManager expansionManager; | ||
| protected readonly IExpansionManager ExpansionManager; |
There was a problem hiding this comment.
Shouldn't both of these be _camelCase (or am I missing some naming convention rule)?
There was a problem hiding this comment.
If it's protected, we tend to do this style since it's acting more like a property.
There was a problem hiding this comment.
@Youssef1313 I found this surprising too, but these were mechanical changes driver by the Roslyn .editorconfig rules so who am I to argue ¯\_(ツ)_/¯
Looks like protected readonly is PascalCase and protected is camelCase with underscore. It doesn't not make sense, I guess, but its a good reminder of why we should all have personal projects we can escape to, with sensible rules :)
| @@ -2,8 +2,6 @@ | |||
| // The .NET Foundation licenses this file to you under the MIT license. | |||
| // See the LICENSE file in the project root for more information. | |||
|
|
|||
| #nullable disable | |||
There was a problem hiding this comment.
Are we able to do this to the VS layer at the same time, since the annotations are likely the same?
There was a problem hiding this comment.
(I'm actually not sure if we can just unify this file, since it seems like it doesn't have any dependencies and each side inherits it appropriately.)
ghost
added this to the 
* upstream/master: (519 commits) Remove workaround in PEMethodSymbol.ExplicitInterfaceImplementations (dotnet#49246) Enable LSP pull model diagnostic for XAML. (dotnet#49145) Update dependencies from https://github.com/dotnet/roslyn build 20201109.8 (dotnet#49240) Add test for with expression in F1 help service (dotnet#49236) Cache RegexPatternDetector per compilation Fix RazorRemoteHostClient to maintain binary back-compat Further tweak inline hints Fix MemberNames API for records (dotnet#49138) Minor cleanups (dotnet#49204) Report warning for assignment or explicit cast of possibly null value of unconstrained type parameter type (dotnet#48803) Clean up of EditorFeatures.Cocoa.Snippets (dotnet#49188) Fix OK button state handling. Make relation between viewmodels more tightly coupled Extend make type abstract to include records (dotnet#48227) Remove duplicated implementations of C# event hookup Add select all/deselect all buttons Consolidate conditional compilation (dotnet#49150) Implement IEquatable on Microsoft.Cci.DefinitionWithLocation structure (dotnet#49162) Add new document extensions file Unify implementations Only disable structure tagger provider in LSP scenario ...
Part of #48871
There is a lot of code removed here, so commit-by-commit is most definitely recommended, if not required.
This is quite a mechanical set of changes, mostly just fixing compiler errors or warnings. I've commented inline on the potentially controversial bit.