Skip to content

Conversation

sigurdm
Copy link
Contributor

@sigurdm sigurdm commented Aug 26, 2025

Not sure if anyone is using this option really. But I think the handling can be simplified.

But it seems that having multiple Generator classes just for deciding whether to write files or not is excessive, and likely to get out of sync (does the EmptyGenerator really still do whatever GeneratorFrontend does, just without writing files?)

@sigurdm sigurdm requested a review from szakarias August 26, 2025 13:01
Copy link
Contributor

@szakarias szakarias left a comment

Choose a reason for hiding this comment

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

Overall looks good, will take another pass tonight.

Copy link
Member

@srawlins srawlins left a comment

Choose a reason for hiding this comment

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

Love it! Definitely simpler!

@sigurdm sigurdm merged commit 435198f into dart-lang:main Aug 26, 2025
11 checks passed
@szakarias
Copy link
Contributor

I have a couple of nits, I will add it in a follow-up PR.

Also, I was thinking of a slightly different design, where we might want to keep the Generator class abstract. I'll put it in a draft for discussion. :-)

szakarias added a commit that referenced this pull request Aug 28, 2025
set generator(Generator newGenerator) => _generator = newGenerator;

/// Factory method that builds Dartdoc with an empty generator.
factory Dartdoc.withEmptyGenerator(
Copy link
Member

Choose a reason for hiding this comment

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

FYI, the dart tool is using this in its implementation of dart doc; see https://github.com/dart-lang/sdk/blob/main/pkg/dartdev/lib/src/commands/doc.dart#L119.

I'm sure its possible to update the tool for this change, but we'll want to do that as part of the next roll of this repo into the sdk.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I was unaware of that. I'll make a migration CL...

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha; tracking the work here so we don't lose track: #4100

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