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.

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

Extend make type abstract to include records #48227

Merged
merged 28 commits into from
Nov 5, 2020
Merged

Extend make type abstract to include records #48227

merged 28 commits into from
Nov 5, 2020

Conversation

Youssef1313
Copy link
Member

Fixes #48129

@Youssef1313 Youssef1313 requested a review from a team as a code owner October 1, 2020 22:00
@Youssef1313 Youssef1313 mentioned this pull request Oct 1, 2020
Closed
Youssef1313
Youssef1313 commented Oct 1, 2020
View reviewed changes
...atures/VisualBasic/Portable/MakeClassAbstract/VisualBasicMakeClassAbstractCodeFixProvider.vb Outdated
@@ -6,13 +6,13 @@ Imports System.Collections.Immutable
Imports System.Composition
Imports System.Diagnostics.CodeAnalysis
Imports Microsoft.CodeAnalysis.CodeFixes
Imports Microsoft.CodeAnalysis.MakeClassAbstract
Imports Microsoft.CodeAnalysis.MakeTypeAbstract
Imports Microsoft.CodeAnalysis.VisualBasic.Syntax

Namespace Microsoft.CodeAnalysis.VisualBasic.MakeClassAbstract
<ExportCodeFixProvider(LanguageNames.VisualBasic, Name:=NameOf(VisualBasicMakeClassAbstractCodeFixProvider)), [Shared]>
Friend NotInheritable Class VisualBasicMakeClassAbstractCodeFixProvider
Copy link
Member Author

Choose a reason for hiding this comment

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

Do you want to update the VB class name as well?

Copy link
Member

Choose a reason for hiding this comment

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

I would say yes for consistency.

davidwengier
davidwengier approved these changes Oct 1, 2020
View reviewed changes
Copy link
Contributor

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

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

Couple of minor test comments, but LGTM in general.

If you're bored, a test for a record with a primary constructor might be nice to ensure no regressions, but I'd be very surprised if it caught anything :)

src/EditorFeatures/CSharpTest/MakeTypeAbstract/MakeTypeAbstractTests.cs Outdated Show resolved Hide resolved
src/EditorFeatures/CSharpTest/MakeTypeAbstract/MakeTypeAbstractTests.cs Outdated Show resolved Hide resolved
@Youssef1313
Copy link
Member Author

@davidwengier Thanks for review. I addressed your feedback. (Sorry for having much commits, working from web browser currently 😄)

This was referenced Oct 2, 2020
Closed
Closed
Youssef1313
Youssef1313 commented Oct 2, 2020
View reviewed changes
src/Features/Core/Portable/MakeClassAbstract/AbstractMakeTypeAbstractCodeFixProvider.cs
editor.ReplaceNode(classDeclaration,
(currentClassDeclaration, generator) => generator.WithModifiers(currentClassDeclaration, DeclarationModifiers.Abstract));
editor.ReplaceNode(typeDeclaration,
(currentTypeDeclaration, generator) => generator.WithModifiers(currentTypeDeclaration, DeclarationModifiers.Abstract));
Copy link
Member Author

Choose a reason for hiding this comment

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

This is not working due to lack of support for records in syntax generator.
Being fixed in #48096

CyrusNajmabadi
CyrusNajmabadi reviewed Oct 4, 2020
View reviewed changes
src/Features/CSharp/Portable/MakeTypeAbstract/CSharpMakeTypeAbstractCodeFixProvider.cs Outdated
@@ -53,15 +50,13 @@ protected override bool IsValidRefactoringContext(SyntaxNode? node, [NotNullWhen
return false;
}

var enclosingType = node.FirstAncestorOrSelf<TypeDeclarationSyntax>();
if (!enclosingType.IsKind(SyntaxKind.ClassDeclaration))
typeDeclaration = node.FirstAncestorOrSelf<TypeDeclarationSyntax>();
Copy link
Member

Choose a reason for hiding this comment

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

don't do this. you will assign the value, even when returning false. it would be far cleaner to keep the out-value 'null' when 'false' is returned.

CyrusNajmabadi
CyrusNajmabadi reviewed Oct 4, 2020
View reviewed changes
src/EditorFeatures/CSharpTest/MakeTypeAbstract/MakeTypeAbstractTests.cs Outdated
[Theory, Trait(Traits.Feature, Traits.Features.CodeActionsMakeTypeAbstract)]
[InlineData("class")]
[InlineData("record")]
public async Task TestMethod(string typeKind)
Copy link
Member

Choose a reason for hiding this comment

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

can you revert all of this add only new tests for records. parameterizing here seems to provide no value and doubles the amount of tests without need.

@davidwengier
Copy link
Contributor

@Youssef1313 is this blocked on anything still?

@Youssef1313 Youssef1313 requested a review from a team as a code owner November 5, 2020 10:59
@Youssef1313
Copy link
Member Author

Youssef1313 commented Nov 5, 2020 •
edited
Loading

@Youssef1313 is this blocked on anything still?

@davidwengier I don't think. Let's see if the tests will pass.

@Youssef1313
Copy link
Member Author

@davidwengier Can you re-run the failed job? The failure is an unrelated test timeout:

Roslyn Error: test timeout exceeded, dumping remaining processes
Dumping dotnet 5320 to F:\workspace\_work\1\s\artifacts\log\Debug\dotnet-1.dmp ... succeeded (129216285 bytes)
Dumping dotnet 5668 to F:\workspace\_work\1\s\artifacts\log\Debug\dotnet-2.dmp ... succeeded (109926677 bytes)
Dumping testhost.net472.x86 5612 to F:\workspace\_work\1\s\artifacts\log\Debug\testhost.net472.x86-3.dmp ... succeeded (701572436 bytes)
##[error](NETCORE_ENGINEERING_TELEMETRY=Test) Tests failed

@jcouv jcouv added the Feature - Records Records label Nov 5, 2020
@davidwengier
Copy link
Contributor

Thanks @Youssef1313

@davidwengier davidwengier merged commit c73027c into dotnet:master Nov 5, 2020
@ghost ghost added this to the Next milestone Nov 5, 2020
@Youssef1313 Youssef1313 deleted the patch-38 branch November 6, 2020 02:39
shyamnamboodiripad pushed a commit that referenced this pull request Nov 6, 2020
@Youssef1313 @shyamnamboodiripad
Extend make type abstract to include records (#48227)
2959366
333fred added a commit to 333fred/roslyn that referenced this pull request Nov 9, 2020
@333fred
Merge remote-tracking branch 'upstream/master' into merge-master
06dc743 
* 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
  ...
@allisonchou allisonchou modified the milestones: Next, 16.9.P2 Nov 24, 2020
@Youssef1313 Youssef1313 mentioned this pull request Feb 18, 2021
Closed
92 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Evangelink Evangelink Evangelink left review comments

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi left review comments

@davidwengier davidwengier davidwengier approved these changes

Assignees

@davidwengier davidwengier

Labels
Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. Feature - Records Records
Projects
None yet
Milestone
16.9.P2
Development

Successfully merging this pull request may close these issues.

CSharpMakeClassAbstractCodeFixProvider needs to be updated for records
7 participants
@Youssef1313 @davidwengier @CyrusNajmabadi @Evangelink @jinujoseph @jcouv @allisonchou