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

Modernize to Incremental Source Generators #132

Merged
merged 8 commits into from
Nov 10, 2022
Merged

Modernize to Incremental Source Generators #132

merged 8 commits into from
Nov 10, 2022

Conversation

viceroypenguin
Copy link
Contributor

No description provided.

@CLAassistant
Copy link

CLAassistant commented Nov 9, 2022

CLA assistant check
All committers have signed the CLA.

@viceroypenguin viceroypenguin marked this pull request as ready for review November 9, 2022 19:59
@viceroypenguin
Copy link
Contributor Author

@kzu This PR strictly updates all generators to use incremental generator technology. Other PRs will follow.

@kzu
Copy link
Member

kzu commented Nov 9, 2022

To fix the dotnet-format check, just run dotnet format on the repo root and commit the changes

@kzu
Copy link
Member

kzu commented Nov 9, 2022

Hey @viceroypenguin , thanks a lot for tackling this! I've been meaning to spend time on it for months now, he. So glad you could work on it!

It looks really nice and focused on just the conversion, which is very much appreciated.

@viceroypenguin
Copy link
Contributor Author

To fix the dotnet-format check, just run dotnet format on the repo root and commit the changes

Done

@kzu
Copy link
Member

kzu commented Nov 9, 2022

Ok, just rebased on main to get minor changes from there

@viceroypenguin
Copy link
Contributor Author

You bet! Finally looking into doing ThisAssembly.EmbeddedResources, so cleaning things up first seemed to make sense.

@viceroypenguin
Copy link
Contributor Author

@kzu - unrelated: would you be opposed to me changing the SourceItemType of the items for ThisAssembly.Strings? "EmbeddedResource" would be accurate for ThisAssembly.EmbeddedResource, while "Strings" or "ResourceString" would be more appropriate for ThisAssembly.Strings.

@kzu
Copy link
Member

kzu commented Nov 9, 2022

Wouldn't both generators be able to work off of the same SourceItemType of EmbeddedResource? The underlying generator would require a run-time embedded resource anyway, right? If so, I think the new generator could leverage the same item type as the Strings one?

@viceroypenguin
Copy link
Contributor Author

viceroypenguin commented Nov 9, 2022

Couple factors at play here:

  • .Strings wants to import the text of the embedded resource so that it can do XML parsing. This means the full text of the resource is part of the incremental cache - it would be better for .Strings not to accidentally include .EmbeddedResources (knowing at some level that it won't due to the add'l metadata filter in StringsGenerator.cs)
  • .EmbeddedResources probably does not want to import the resx files - generally those will get accessed via ResourceManager instead of EmbeddedResources since the .resx file itself is not important to the consumer. Though if it did, it really wouldn't be a significant effect.
  • The .Strings target will possibly import EmbeddedResource twice for .resx files, since they will be included via the .Strings target and the .EmbeddedResources target

@kzu
Copy link
Member

kzu commented Nov 9, 2022

Ah, yeah, I understand the difference now.
Yeah, changing the Strings one makes sense. Thanks for your patience in clarifying!

@viceroypenguin viceroypenguin changed the title [WIP] Modernize to Incremental Source Generators Modernize to Incremental Source Generators Nov 9, 2022
@kzu kzu merged commit 9189601 into devlooped:main Nov 10, 2022
@viceroypenguin viceroypenguin deleted the modernize branch November 10, 2022 14:37
@kzu kzu added the enhancement New feature or request label Dec 31, 2022
@devlooped devlooped locked and limited conversation to collaborators Sep 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants