Skip to content

Add multiple partial class definition support for embed font generator#43

Merged
drewnoakes merged 10 commits intomasterfrom
embed-font-generator-partial-class-support
Jun 14, 2025
Merged

Add multiple partial class definition support for embed font generator#43
drewnoakes merged 10 commits intomasterfrom
embed-font-generator-partial-class-support

Conversation

@jonathanou
Copy link
Copy Markdown
Collaborator

Currently when multiple partial class definitions of the same class use [EmbedFiggleFont], multiple generated files of the same filenames are produced and the compiler will error due conflicting filenames.

This PR enables support for using EmbedFiggleFont on multiple partial class definitions of the same class name and closes #35 .

Implementation

  • Extract the attribute info aggregation code from RenderTextSourceGenerator (which does support multiple partial definitions) to an extension method for reuse.
  • Update EmbedFontSourceGenerator to use the new extension method.
  • Also extracted an extension method for creating GenerationInfo from attributes on class declarations.
  • Updated both generators to use the new extension method to reduce duplicate code.

Testing

  • Added new unit tests for EmbedFontSourceGenerator to check multiple partial definitions are working as expected.
  • All existing tests continue to pass after extension methods are extracted.

@jonathanou jonathanou requested review from Copilot and drewnoakes June 9, 2025 14:56
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors attribute-aggregation logic into reusable extension methods and updates both source generators to support multiple partial class definitions for embedding Figgle fonts, avoiding filename conflicts.

  • Extracted attribute collection and consolidation into ForFiggleAttributeWithMetadataName and ConsolidateAttributeInfosByTypeSymbol extension methods.
  • Updated RenderTextSourceGenerator and EmbedFontSourceGenerator to use the new extensions and merge multiple partial definitions.
  • Added unit tests in EmbedFontSourceGeneratorTests to verify behavior with multiple partial definitions and duplicate attributes.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
Figgle.Generator/RenderTextSourceGenerator.cs Replaced inline attribute aggregation with reusable extensions and adjusted the source-output loop for multiple partial definitions.
Figgle.Generator/IncrementalGeneratorExtensions.cs Added new extension methods: ForFiggleAttributeWithMetadataName and ConsolidateAttributeInfosByTypeSymbol.
Figgle.Generator/IncrementalGeneratorContextExtensions.cs Removed duplicate external-fonts provider now in IncrementalGeneratorExtensions.
Figgle.Generator/GenerationInfo.cs Introduced generic GenerationInfo<TAttributeInfo> record struct.
Figgle.Generator/EmbedFontSourceGenerator.cs Switched to consolidated attribute info, iterated multiple partials, and updated file naming.
Figgle.Generator.Tests/EmbedFontSourceGeneratorTests.cs Added tests for duplicate attributes and member names across partial definitions.
Comments suppressed due to low confidence (3)

Figgle.Generator/EmbedFontSourceGenerator.cs:131

  • Using return inside the loop will abort processing of all types after an invalid type; use continue here to skip only the invalid targetType and proceed with others.
return;

Figgle.Generator/EmbedFontSourceGenerator.cs:204

  • Using ToDisplayString(_fullyQualifiedFormat) for the generated filename may introduce characters invalid for file paths; consider using targetType.Name or sanitizing the string.
"${targetType.ToDisplayString(_fullyQualifiedFormat)}.g.cs",

Figgle.Generator/RenderTextSourceGenerator.cs:128

  • [nitpick] The variable name embedFontInfoProvider is misleading in RenderTextSourceGenerator; consider renaming to something like renderTextInfoProvider to reflect its purpose.
var embedFontInfoProvider = generationInfosProvider.Combine(externalFontsProvider);

{
partial class DemoUsage
{
public static string FiggleString { get; } = @"_____________________________ _______
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

NOTE: due to the use of HashSet when merging attributes, the order of the generated properties can be non-deterministic and was causing intermittent test failures.

To fix this, I switched to using ImmutableSortedSet and compared the items using their member names; thus the generated properties are always in alphabetical order so we must adjust our test code to match.

@drewnoakes
Copy link
Copy Markdown
Owner

Been flat out today. Will review tomorrow 👍

Copy link
Copy Markdown
Owner

@drewnoakes drewnoakes left a comment

Choose a reason for hiding this comment

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

LGTM!

What's the best sequence to avoid conflicts between this and #42?

@jonathanou
Copy link
Copy Markdown
Collaborator Author

LGTM!

What's the best sequence to avoid conflicts between this and #42?

I think it might be easier if this PR went in first because git might be able to auto merge since it knows the new file is renamed? Let me know if this isn't the case, I can wait for your changes and re-apply mine.

@drewnoakes drewnoakes merged commit ae29d32 into master Jun 14, 2025
2 checks passed
@drewnoakes drewnoakes deleted the embed-font-generator-partial-class-support branch June 14, 2025 16:18
@drewnoakes
Copy link
Copy Markdown
Owner

Tried the merge locally both ways and merging this first seems easier.

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.

EmbedFontSourceGenerator doesn't support multiple partial definitions of the same target type

3 participants