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.

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

Exposed contexts as interfaces. Exposed original IncrementalGenerator… #30

Closed
wants to merge 1 commit into from

Conversation

scooter12
Copy link
Contributor

Exposed contexts as interfaces. Exposed original IncrementalGeneratorInitializationContext and ILogger via prop-get with intellisense warnings. closes #29

…InitializationContext and ILogger via prop-get with intellisense warnings.
/// </summary>
/// <remarks>Provided as a convenience accessor to support advanced scenarios. Generally should not be directly
/// interacted with as it may produce side effects in the sgf generators. Use at your own risk.</remarks>
IncrementalGeneratorInitializationContext Context { get; }
Copy link
Owner

Choose a reason for hiding this comment

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

I think it would be better to hide this from the interface and instead add it to the implementation then update the generators to take in ISgfInitializationContext instead.

This

public abstract void OnInitialize(SgfInitializationContext context);

would become

public abstract void OnInitialize(ISgfInitializationContext context);

The reason being is that people don't often read the documentation and learn from intelisense. If we expose the property they might use it and when they do they loose all the exception handling that SGF provides. There are for sure going to be use cases where you need it but for folks who really need it can read the docs or find out how to get it will a little digging.

@ByronMayne
Copy link
Owner

I ended up making the changes here #38. The only real difference is I hid the base context. You have to do an explicit cast to the interface to get access to it.

@ByronMayne ByronMayne closed this Dec 24, 2024
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.

Use interfaces on publicly visible classes and structs to support dependency injection
2 participants