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.

Sign up for GitHub

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

Jump to bottom

Move templating to the client side. #76

Merged
merged 7 commits into from
Oct 10, 2024
Merged

Move templating to the client side. #76

merged 7 commits into from
Oct 10, 2024

Conversation

davidmorgan
Copy link
Contributor

@davidmorgan davidmorgan commented Sep 30, 2024 •
edited
Loading

Pretty sure there's more to add to the Augmentation model representation for other TODOs, but this PR just adds the simplest things that allows the templating to move client side: a split into "resolved strings" and identifiers.

@davidmorgan davidmorgan force-pushed the client-side-templating branch from 9420577 to 2576863 Compare September 30, 2024 15:39
@davidmorgan davidmorgan force-pushed the client-side-templating branch from 2576863 to ffb5cfe Compare September 30, 2024 16:03
@davidmorgan davidmorgan marked this pull request as ready for review September 30, 2024 16:10
jakemac53
jakemac53 reviewed Sep 30, 2024
View reviewed changes
pkgs/_analyzer_macros/lib/macro_implementation.dart Outdated Show resolved Hide resolved
pkgs/_cfe_macros/lib/macro_implementation.dart Outdated Show resolved Hide resolved
pkgs/_macro_host/test/macro_host_test.dart Outdated Show resolved Hide resolved
pkgs/_test_macros/lib/declare_x_macro.dart Outdated Show resolved Hide resolved
schemas/dart_model.schema.json Outdated Show resolved Hide resolved
@davidmorgan davidmorgan mentioned this pull request Oct 2, 2024
Open
@davidmorgan davidmorgan requested a review from jakemac53 October 8, 2024 15:08
jakemac53
jakemac53 reviewed Oct 8, 2024
View reviewed changes
Copy link
Contributor

@jakemac53 jakemac53 left a comment

Choose a reason for hiding this comment

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

Previous comments still stand regarding ResolvedCode and moving the template expansion completely out to the JSON macro itself

pkgs/_analyzer_macros/lib/macro_implementation.dart Outdated
@@ -9,7 +9,7 @@ import 'package:analyzer/src/summary2/macro_declarations.dart' as analyzer;
import 'package:analyzer/src/summary2/macro_injected_impl.dart' as injected;
import 'package:dart_model/dart_model.dart';
import 'package:macro_service/macro_service.dart';
import 'package:macros/macros.dart' hide Code;
import 'package:macros/macros.dart' as injected;
Copy link
Contributor

Choose a reason for hiding this comment

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

I generally would prefer prefixes named after the package, so just macros in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmmm are you sure? This is intentionally the same as the macro_injected_impl.dart and executor.dart prefixes, so it's easy to pick out all the types destined for injection into the analyzer. Whereas "macros" would not say which of the two macros worlds it's from :)

Copy link
Contributor

Choose a reason for hiding this comment

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

The word injected I think in this case is also just super confusing to me? I think because the whole situation is so complicated, I don't really understand what is injected into what or how, and so this term is just confusing... at least for macros I know its the package:macros stuff?

Copy link
Contributor

Choose a reason for hiding this comment

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

I also did not realize we were using overlapping prefixes, that is just even more confusing haha. I would call the other one analyzer_macros or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From discussion: macros_api_v1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

davidmorgan
davidmorgan commented Oct 8, 2024
View reviewed changes
Copy link
Contributor Author

@davidmorgan davidmorgan left a comment

Choose a reason for hiding this comment

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

Whoops, I had pending comments that I failed to publish, that's why you didn't get the review again. My bad.

I dropped them since we already discussed since I wrote those comments, what I'm publishing now is brand new fresh comments, PTAL ;)

pkgs/_test_macros/lib/declare_x_macro.dart Outdated Show resolved Hide resolved
pkgs/_analyzer_macros/lib/macro_implementation.dart Outdated
@@ -9,7 +9,7 @@ import 'package:analyzer/src/summary2/macro_declarations.dart' as analyzer;
import 'package:analyzer/src/summary2/macro_injected_impl.dart' as injected;
import 'package:dart_model/dart_model.dart';
import 'package:macro_service/macro_service.dart';
import 'package:macros/macros.dart' hide Code;
import 'package:macros/macros.dart' as injected;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmmm are you sure? This is intentionally the same as the macro_injected_impl.dart and executor.dart prefixes, so it's easy to pick out all the types destined for injection into the analyzer. Whereas "macros" would not say which of the two macros worlds it's from :)

schemas/dart_model.schema.json Outdated Show resolved Hide resolved
@davidmorgan davidmorgan requested a review from jakemac53 October 8, 2024 19:52
@davidmorgan
Copy link
Contributor Author

Notes from our discussion:

ResolvedCode -> Text. (Or: check if we can have String in a union).

@davidmorgan davidmorgan force-pushed the client-side-templating branch from 42a0844 to dcdd740 Compare October 10, 2024 07:42
@davidmorgan davidmorgan force-pushed the client-side-templating branch from dcdd740 to b6498e4 Compare October 10, 2024 07:55
@davidmorgan davidmorgan merged commit 8c4b89e into dart-lang:main Oct 10, 2024
45 checks passed
@davidmorgan davidmorgan deleted the client-side-templating branch October 10, 2024 08:09
@davidmorgan davidmorgan mentioned this pull request Oct 10, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakemac53 jakemac53 jakemac53 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@davidmorgan @jakemac53