Skip to content

Conversation

@nojaf
Copy link
Contributor

@nojaf nojaf commented Sep 12, 2022

A less impactful version of #13855.
In this PR I'm only adding the signature parameter representation, not touching anything related to SynArgInfo.

@nojaf nojaf marked this pull request as ready for review September 12, 2022 14:24
@nojaf
Copy link
Contributor Author

nojaf commented Sep 12, 2022

Yes, let's totally take this one, @dsyme ready for review.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please also adjust

  • traverseSynType in ServiceParseTreeWalk
  • walkType in GetEntityKind (what is that funciton anyway? It has so much code walking duplication!)

Maybe also remove the default | _ -> pattern matching in those two cases so these missing cases would have been discovered, and check for other missing cases there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I've addressed the missing cases.

Copy link
Contributor

@dsyme dsyme left a comment

Choose a reason for hiding this comment

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

Just a couple of changes needed, looks really good otherwise

@nojaf nojaf force-pushed the syntype-signature-parameter branch from 96cd1a4 to a9b956b Compare September 13, 2022 13:14
@nojaf nojaf requested a review from dsyme September 13, 2022 13:36
@dsyme dsyme merged commit 8f59d36 into dotnet:main Sep 14, 2022
vzarytovskii added a commit that referenced this pull request Sep 14, 2022
* Introduce SynType.SignatureParameter.

* Add missing SynType cases in walkers.

Co-authored-by: Florian Verdonck <[email protected]>
Co-authored-by: Vlad Zarytovskii <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants