Skip to content

Offer property attribute target in record positional param#51760

Merged
CyrusNajmabadi merged 5 commits intodotnet:mainfrom
Youssef1313:record-attribute-target-completion
Apr 17, 2021
Merged

Offer property attribute target in record positional param#51760
CyrusNajmabadi merged 5 commits intodotnet:mainfrom
Youssef1313:record-attribute-target-completion

Conversation

@Youssef1313
Copy link
Member

@Youssef1313 Youssef1313 commented Mar 9, 2021 •
edited
Loading

Fixes #51756

image

@Youssef1313 Youssef1313 requested a review from a team as a code owner March 9, 2021 15:12
@ghost ghost added the Area-IDE label Mar 9, 2021
@Youssef1313
Copy link
Member Author

Youssef1313 commented Mar 9, 2021

Closing and re-opening for a new build due to unrelated failure:

C:\Users\VssAdministrator.nuget\packages\microsoft.dotnet.helix.sdk\2.0.0-beta.21123.5\tools\azure-pipelines\AzurePipelines.MultiQueue.targets(43,5): error : {"$id":"1","innerException":null,"message":"TF10216: Azure DevOps services are currently unavailable. Try again later. Activity Id: d549ace2-75f6-4e49-9123-dbe13c215d0a","typeName":"Microsoft.VisualStudio.Services.CircuitBreaker.CircuitBreakerShortCircuitException, Microsoft.VisualStudio.Services.Common","typeKey":"CircuitBreakerShortCircuitException","errorCode":0,"eventId":3000} [D:\a\1\s\helix-tmp.csproj]

@Youssef1313 Youssef1313 closed this Mar 9, 2021
@Youssef1313 Youssef1313 reopened this Mar 9, 2021
@Youssef1313
Copy link
Member Author

Youssef1313 commented Mar 9, 2021

Same failure again 😕

@Youssef1313 Youssef1313 closed this Mar 9, 2021
@Youssef1313 Youssef1313 reopened this Mar 9, 2021
CyrusNajmabadi
CyrusNajmabadi reviewed Mar 10, 2021
View reviewed changes
...SharedUtilitiesAndExtensions/Workspace/CSharp/Extensions/ContextQuery/CSharpSyntaxContext.cs Outdated
Comment on lines +326 to +328
var isRecordParameter = token.Parent.IsParentKind(SyntaxKind.Parameter) &&
token.Parent.Parent.IsParentKind(SyntaxKind.ParameterList) &&
token.Parent.Parent.Parent.IsParentKind(SyntaxKind.RecordDeclaration);
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi Mar 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

if (token.Parent is { Parent: { Kind: Parameter, Parent: { Parent: { Kind: RecordDeclaration } } } })
?

Copy link
Member Author

@Youssef1313 Youssef1313 Mar 15, 2021

Choose a reason for hiding this comment

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

@CyrusNajmabadi I'll have to use RawKind for this form as there is no Kind property. The cast to int feels annoying to me. But let me know if this is okay for you.

CyrusNajmabadi
CyrusNajmabadi reviewed Mar 10, 2021
View reviewed changes
...SharedUtilitiesAndExtensions/Workspace/CSharp/Extensions/ContextQuery/CSharpSyntaxContext.cs Outdated
token.Parent.Parent.Parent.IsParentKind(SyntaxKind.RecordDeclaration);

return isRecordParameter || SyntaxTree.IsMemberDeclarationContext(
token.SpanStart, contextOpt: null, validModifiers: null, validTypeDeclarations: validTypeDeclarations, canBePartial: false, cancellationToken: cancellationToken);
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi Mar 10, 2021

Choose a reason for hiding this comment

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

my pref is two if-statements.

Copy link
Member Author

@Youssef1313 Youssef1313 Mar 15, 2021

Choose a reason for hiding this comment

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

Done

CyrusNajmabadi
CyrusNajmabadi approved these changes Mar 10, 2021
View reviewed changes
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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

LMK if you want to make changes. otherwise i'll merge in.

@jinujoseph jinujoseph added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Mar 11, 2021
@Youssef1313 Youssef1313 mentioned this pull request Apr 7, 2021
Closed
92 tasks
@Youssef1313
Copy link
Member Author

Youssef1313 commented Apr 14, 2021

@CyrusNajmabadi Anything left here? Thanks.

@CyrusNajmabadi
Copy link
Contributor

CyrusNajmabadi commented Apr 14, 2021

lgtm. thanks!

auto-merge was automatically disabled April 15, 2021 06:24

Head branch was pushed to by a user without write access

@Youssef1313
Update src/Workspaces/SharedUtilitiesAndExtensions/Workspace/CSharp/E…
0a883bb 
…xtensions/ContextQuery/CSharpSyntaxContext.cs
@Youssef1313 @CyrusNajmabadi
Update src/Workspaces/SharedUtilitiesAndExtensions/Workspace/CSharp/E…
ac65a9d 
…xtensions/ContextQuery/CSharpSyntaxContext.cs

Co-authored-by: CyrusNajmabadi <cyrus.najmabadi@gmail.com>
auto-merge was automatically disabled April 17, 2021 17:08

Head branch was pushed to by a user without write access

@Youssef1313
Copy link
Member Author

Youssef1313 commented Apr 17, 2021

@CyrusNajmabadi Auto merge was cancelled due to pushing a commit to fix formatting. It should hopefully be green now.

@CyrusNajmabadi
Copy link
Contributor

CyrusNajmabadi commented Apr 17, 2021

Thanks!

@CyrusNajmabadi CyrusNajmabadi merged commit 9c67fb1 into dotnet:main Apr 17, 2021
@ghost ghost added this to the Next milestone Apr 17, 2021
@Youssef1313 Youssef1313 deleted the record-attribute-target-completion branch April 17, 2021 18:49
@dibarbet dibarbet modified the milestones: Next, 16.10.P3 Apr 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

16.10.P3

Development

Successfully merging this pull request may close these issues.

Completion don't provide property target in record parameter

4 participants

@Youssef1313 @CyrusNajmabadi @dibarbet @jinujoseph