Skip to content

Conversation

nikeokoronkwo
Copy link
Collaborator

Fixes #374

This Pull Request extends the functionality of IDL generation by allowing support for parsing individual IDL files and definitions.

This also contains an integration test suite integration/idl_test.dart for testing IDL generation against expected files in integration/idl/

For more information, check #374

- [x] I’ve reviewed the contributor guide and applied the relevant portions to this PR.

@nikeokoronkwo nikeokoronkwo marked this pull request as ready for review June 11, 2025 01:33
@nikeokoronkwo nikeokoronkwo requested a review from kevmoo June 11, 2025 04:44
@kevmoo kevmoo requested a review from srujzs June 13, 2025 00:39
Copy link
Contributor

@srujzs srujzs left a comment

Choose a reason for hiding this comment

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

Nice, thanks Nike!

final inputName = inputFileName.replaceFirst('_input', '');

final outputActualPath =
p.join('.dart_tool', 'idl', '${inputName}_actual.dart');
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use a temp directory instead using dart:io.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kevmoo suggested I write it to .dart_tool. It was originally in a temp directory ignored in the .gitignore

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I meant more that we dynamically generate the temp directory and clean it up each time instead of keeping a temp directory alive.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What if we might want to look at the actual output and compare?

Copy link
Contributor

Choose a reason for hiding this comment

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

You'd just comment out the deletion if you're debugging locally. I'm not sure when .dart_tool cleans up any writes to it, maybe @kevmoo knows more there. Keeping alive test artifacts seems like a bad idea though.

final outputActualPath =
p.join('.dart_tool', 'idl', '${inputName}_actual.dart');
final outputExpectedPath =
p.join(testGenFolder, '${inputName}_expected.dart');
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to include these files in this PR? I only see the input IDL files.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't have expected files (for all cases) because the test cases threw errors, and I wanted to resolve them to at least get an idea of how the output would look like before placing expected files

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, I'm guessing they'll be added in this PR at some point later then.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wanted us to discuss about what was going wrong before then

@kevmoo
Copy link
Member

kevmoo commented Jun 17, 2025

@srujzs – running tests. Is this good?

@srujzs
Copy link
Contributor

srujzs commented Jun 18, 2025

We chatted a bit before about some of the failures for the expected files and I believe Nike is still planning on adding the expected outputs into this PR before we can land.

@nikeokoronkwo
Copy link
Collaborator Author

All tests are passing now @srujzs

Copy link
Contributor

@srujzs srujzs left a comment

Choose a reason for hiding this comment

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

Looking good! Just one final nit.

Copy link

auto-submit bot commented Jun 26, 2025

autosubmit label was removed for dart-lang/web/377, because This PR has not met approval requirements for merging. The PR author is not a member of dart-team and needs 1 more review(s) in order to merge this PR.

  • Merge guidelines: A PR needs at least one approved review if the author is already part of flutter-hackers or two member reviews if the author is not a flutter-hacker before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

@srujzs srujzs merged commit c0d73b5 into dart-lang:main Jun 26, 2025
16 checks passed
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.

[web_generator] Support for Single IDL File Parsing
3 participants