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.

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

Small refactorings in tool_runner.dart #2685

Merged
merged 5 commits into from Jun 18, 2021

Conversation

srawlins
Copy link
Member

In debugging #2344, I went down some dead ends but ended up with some refactorings along the way. Some of these may be my personal preference, so are certainly up for discussion.

  • the environment variables in the callback passed to config.tools.runner.run( were quite indented; extracted to a private method.
  • ToolTempFileTracker had a weird or outdated doc comment; I removed it.
  • ToolTempFileTracker.createTemporaryFile did not need to be async; fixed.
  • Add a doc comment to ToolRunner._runProcess; edit doc comment to ToolRunner.run.
  • Make a number of parameters named; in particular, toolErrorCallback is typically given a function literal, so the name helps identify what this function is supposed to be.
  • Extract the code that writes the tool content to a file, into a method.

@google-cla google-cla bot added the cla: yes Google CLA check succeeded. label Jun 17, 2021
@coveralls
Copy link

coveralls commented Jun 17, 2021

Coverage Status

Coverage increased (+0.03%) to 58.089% when pulling 4a763ae on srawlins:tidy-tool-runner into dedab6e on dart-lang:master.

@srawlins srawlins requested a review from jcollins-g June 17, 2021 09:04
Copy link
Contributor

@jcollins-g jcollins-g left a comment

Choose a reason for hiding this comment

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

lgtm, see comment about ignore though

lib/src/tool_runner.dart Outdated Show resolved Hide resolved
@srawlins srawlins merged commit 3cf7c63 into dart-lang:master Jun 18, 2021
@srawlins srawlins deleted the tidy-tool-runner branch June 18, 2021 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Google CLA check succeeded.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants