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

Move source gen samples to the incremental model and source to C# 10 #985

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

lucabol
Copy link
Contributor

@lucabol lucabol commented Apr 27, 2022

This PR encompasses two things:

  1. Move the code to C# 10.
  2. Move the generator model to the incremental model.

Lingering doubts:

  1. Is the incremental model the recommended one, aka is the previous one obsolete?
  2. Is the Readme still a faithful representation of the status?

@lucabol lucabol requested a review from a team as a code owner April 27, 2022 11:21
@chsienki
Copy link
Contributor

@lucabol Thanks! I'll take a detailed look through. From a quick glance I think some of the samples could be more incremental so I'll make sure to focus on how to optimize them.

@@ -7,12 +7,12 @@
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Text;

namespace SourceGeneratorSamples
namespace SourceGeneratorSamples;
Copy link
Member

Choose a reason for hiding this comment

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

❗ This change needs to be in a separate pull request (if at all). Generally, significant style changes like this are only accepted after internal discussion and created by a member of the team at a time that's least likely to interfere with other work in progress.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am a bit confused. I am in an email thread with the team members about these changes and everyone is aware of them. I would hate to have done all this work for nothing. I will forward the thread to you. Please discuss with them and let me know how to proceed.

@sharwell
Copy link
Member

sharwell commented May 3, 2022

I think it would be best to resubmit a conversion of just one of the source generators. The change should emphasize the change from ISourceGenerator to IIncrementalGenerator, and omit all other style-type changes. I have so many objections to the style changes in this pull request I do not see any way for it to proceed in its current form.

@sharwell
Copy link
Member

sharwell commented May 5, 2022

I pushed a small change to the generators to fix the generated file names. It should be a relatively easy merge (and you can use a828850 as a reference if you want).

Comment on lines +132 to +135
var collected = fields.Collect()
.Combine(context.CompilationProvider);

context.RegisterSourceOutput(collected, GenerateCode);
Copy link
Member

Choose a reason for hiding this comment

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

That's not incremental at all, I think.

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.

4 participants