Skip to content

Add a generic run_dart_tool command #210

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.

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

Merged
merged 3 commits into from
Jan 7, 2019

Conversation

stuartmorgan-g
Copy link
Collaborator

This is a generic version of the existing update_flutter_engine wrappers,
so that as more dart tools are added they don't each need a wrapper.

update_flutter_engine remains for now, but may be removed in the future
in favor of calling 'run_dart_tool update_flutter_engine' instead. The shell
script version has been changed to match the existing batch script
behavior of automatically providing --flutter_root (since now there's an
easy way for a caller who wants to provide something else to do so).
This may simplify future changes to provide a project-level engine
override (#160)

Copy link
Contributor

@krisgiesing krisgiesing left a comment

Choose a reason for hiding this comment

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

LGTM. The %~dp0 syntax threw me off a bit; I had to look that up. I wonder if it's worth adding a comment about what that does, or if this is just something batch file readers are expected to know? But overall looks good.

@stuartmorgan-g
Copy link
Collaborator Author

It's used in all our existing batch files; I was assuming it was relatively standard. But since I'm new to batch scripting, I don't really have any context. (It seems much less crazy to me that using for to set a variable!)

Once things settle a bit with the new policy to minimize batch/shell in favor of Dart, I'll see how often they need to be touched. Hopefully they will need little to no ongoing maintenance since they will be so small, but if not I can add a bunch of comments to make it easier to maintain them regardless of familiarity level.

@stuartmorgan-g stuartmorgan-g merged commit cf3a791 into google:master Jan 7, 2019
@stuartmorgan-g stuartmorgan-g deleted the run-dart-tool branch January 7, 2019 23:26
@krisgiesing
Copy link
Contributor

Yeah, it does seem standard for batch files. Given the nature of the project, we may have maintainers who are not so familiar with batch (including future-us), so it may be worth adding comments for that reason, even if it's a well known idiom among people who do this a lot.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants