Skip to content

Add AspNetCoreCSharpVirtualCharService#61673

Merged
CyrusNajmabadi merged 3 commits intomainfrom
jamesnk/virtualcharservice-static-property
Jun 3, 2022
Merged

Add AspNetCoreCSharpVirtualCharService#61673
CyrusNajmabadi merged 3 commits intomainfrom
jamesnk/virtualcharservice-static-property

Conversation

@JamesNK
Copy link
Copy Markdown
Member

@JamesNK JamesNK commented Jun 3, 2022

Need to access virtual chars from more places than just classifier: also unit tests and analyzer.

PR removes virtual chars property from context and adds a static instance that can be used anywhere. It's ok that this is tied to CSharp as VB currently isn't a goal. If VB is desired then we can consider a more complicated approach in the future, but this type will still be useful for getting virtual chars in unit tests.

@JamesNK JamesNK requested a review from CyrusNajmabadi June 3, 2022 00:10
@JamesNK JamesNK requested review from a team as code owners June 3, 2022 00:10
@ghost ghost added the Area-IDE label Jun 3, 2022
}

/// <inheritdoc cref="CSharpVirtualCharService.Instance"/>
public static AspNetCoreCSharpVirtualCharService Instance => _instance;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This could be a field but a property gives more flexibility in case the underlying implementation needs to change.

public int Value => _virtualChar.Value;

/// <inheritdoc/>
public override string ToString() => _virtualChar.ToString();
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Adding this at the same time to make debugging a little easier.

internal sealed class AspNetCoreCSharpVirtualCharService
{
private static readonly AspNetCoreCSharpVirtualCharService _instance =
new AspNetCoreCSharpVirtualCharService(CSharpVirtualCharService.Instance);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i just want to check with @sharwell that this is ok. Specifically that an EA layer is linking directly to a C# layer value.

Copy link
Copy Markdown
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.

LGTM.

@CyrusNajmabadi CyrusNajmabadi merged commit 85103ec into main Jun 3, 2022
@CyrusNajmabadi CyrusNajmabadi deleted the jamesnk/virtualcharservice-static-property branch June 3, 2022 02:09
@ghost ghost added this to the Next milestone Jun 3, 2022
@RikkiGibson RikkiGibson modified the milestones: Next, 17.3 P3 Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants