Skip to content
This repository has been archived by the owner on Feb 4, 2025. It is now read-only.

update tool/generate_* scripts and sync files generated by those scripts #150

Merged
merged 5 commits into from
Dec 11, 2024

Conversation

eEQK
Copy link
Contributor

@eEQK eEQK commented Dec 4, 2024

there are some changes I'm not sure should be there? see my comments

cc @davidmorgan

tool/generate_converter Show resolved Hide resolved
tool/generate_converter Show resolved Hide resolved
tool/dart_model_generator/lib/generate_dart_model.dart Outdated Show resolved Hide resolved
tool/generate_converter Show resolved Hide resolved
@eEQK eEQK marked this pull request as draft December 4, 2024 22:01
Copy link
Contributor

@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.

Thanks! Looking good so far.

pkgs/_analyzer_cfe_macros/lib/metadata_converter.dart Outdated Show resolved Hide resolved
tool/dart_model_generator/lib/generate_dart_model.dart Outdated Show resolved Hide resolved
tool/generate_converter Show resolved Hide resolved
tool/generate_converter Show resolved Hide resolved
pkgs/dart_model/lib/src/macro_metadata.g.dart Show resolved Hide resolved
@davidmorgan
Copy link
Contributor

Looks good to me--is it ready for review/merge? Thanks!

@eEQK
Copy link
Contributor Author

eEQK commented Dec 10, 2024

I didn't fix the TODO you've mentioned on discord https://github.com/dart-lang/macros/blob/66baf8b8266d393d2034f02d898723e9318f3ab4/pkgs/_test_macros/lib/built_value.dart#L186

But if I can do it separately then yes the PR is ready!

@davidmorgan
Copy link
Contributor

Hmmm it should be a trivial change now I think--just check the name? Then, it's a good e2e test of the PR :)

Or if it's more work, sure, another PR sounds good.

@eEQK eEQK requested a review from davidmorgan December 10, 2024 11:30
@eEQK eEQK marked this pull request as ready for review December 10, 2024 11:30
@eEQK
Copy link
Contributor Author

eEQK commented Dec 10, 2024

should be ok now, I've also updated the test case

@eEQK eEQK force-pushed the update-definitions branch from e0d92e2 to adb2a64 Compare December 10, 2024 11:47
@eEQK eEQK requested a review from davidmorgan December 10, 2024 11:51
@davidmorgan
Copy link
Contributor

No worries, that's what we have presubmit checks for :) let's see if they turn green this time...

@eEQK
Copy link
Contributor Author

eEQK commented Dec 10, 2024

Fixed the typo and updated goldens, I did run mono_repo analyze && test and I do have a single test failing in macros/pkgs/_macro_tool but I hope it's just a local thing because it fails on main for me as well

❯ dart test
Building package executable...
Built test:test.
00:06 +1 -1: test/macro_runner_test.dart: CfeMacroRunner runs macros [E]
  Crash when compiling:
  'package:front_end/src/source/source_loader.dart': Failed assertion: line 1482 pos 9: '_compilationUnits.values.every((compilationUnit) =>
              !(compilationUnit is SourceCompilationUnit &&
                  compilationUnit.isAugmenting))': Augmentation library found in libraryBuilders: Instance of 'DillCompilationUnitImpl', Instance of 'DillCompilationUnitImpl', ...

@eEQK eEQK requested a review from davidmorgan December 10, 2024 13:36
@davidmorgan
Copy link
Contributor

Looks great, thanks :)

@davidmorgan davidmorgan merged commit a88aa0b into dart-archive:main Dec 11, 2024
51 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants