Follow syntaxfacts pattern#56351
Conversation
…ices/SyntaxFacts/ISyntaxFacts.cs
…ices/SyntaxFacts/ISyntaxFacts.cs
…ices/SyntaxFacts/ISyntaxFacts.cs
…ices/SyntaxFacts/ISyntaxFacts.cs
…ic/Services/SyntaxFacts/VisualBasicSyntaxFacts.vb
…into syntaxFacts
ryzngard
left a comment
There was a problem hiding this comment.
Changes overall look good. Comments on spec written in ISyntaxFacts. I think this should go through a quick review for the whole team to make sure we agree on all of these.
| /// <item> | ||
| /// Functions which are effectively specific to a single feature are are just trying to find a place to place complex | ||
| /// feature logic in a place such that it can run over VB or C#. For example, a function to determine if a position | ||
| /// is on the 'header' of a node. a 'header' is a not a well defined syntax concept that can be trivially asked of |
There was a problem hiding this comment.
Should we note that these could go into IRefactoringHelpersService if we want common definitions for abstract concepts like this?
There was a problem hiding this comment.
i actually am pretty vehemently agqainst the name 'refactoring helpers' as that itself is just vague and leads to kitchen-sink bucketing of things. The current IRefactoringHelpersService should really have a name like IDetermineSelectedNodeService to keep it very well scoped exactly to the concept it can provide. For example, IRefactoringHelpersService shouldn't have things related to formatting/cleanup/code-modification/etc. where 'Refactoring Helpers' might imply that it does.
| /// some other feature specific service. | ||
| /// </item> | ||
| /// <item> | ||
| /// Functions that a single item when one language may allow for multiple. For example 'GetIdentifierOfVariableDeclarator'. |
There was a problem hiding this comment.
Should we specify for these cases that allowing multiple returns is okay? In the example VB has multiple and C# has one. It seems likely if the case is that multiple can ever be returned they should be handled, but 1 and 0 items would also need to be handled.
There was a problem hiding this comment.
Yup. That seems reasonable.
| /// </list> | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// Many helpers in this type currently violate the above 'dos' and 'do nots'. They should be removed and either |
There was a problem hiding this comment.
What's the plan for this? Did you audit functions?
There was a problem hiding this comment.
What's the plan for this?
I have no plan currently. Perhaps over time i (and others) can address deviations.
There was a problem hiding this comment.
I guess unless we're going to have a plan to address this...I'm not sure what the point is in setting guidance we don't meet and don't ever plan to meet.
There was a problem hiding this comment.
I'd make an alternate suggestion (and mentioned this to Cyrus privately): let's at least go through the current methods and stick TODO comments for which ones we think are violating but aren't immediately easy to fix in this PR. The challenge with this comment is if I'm somebody new to the codebase, and I read this, but I also see a bunch of violations, I don't know which is wrong. I can see something is wrong, but I don't know what.
…ices/SyntaxFacts/ISyntaxFacts.cs Co-authored-by: Sam Harwell <sam@tunnelvisionlabs.com>
I recommend starting the review at: https://github.com/dotnet/roslyn/pull/56351/files#diff-f8a376be87a8123f285a06533c2e3fff86d3b0a36044b1f398fe0ee75738b375R77