Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: warn when clang-format fails #62

Merged
merged 6 commits into from
Oct 19, 2017
Merged

feat: warn when clang-format fails #62

merged 6 commits into from
Oct 19, 2017

Conversation

kjin
Copy link
Contributor

@kjin kjin commented Oct 13, 2017

No description provided.

@ofrobots
Copy link
Contributor

Odd that travis wasn't launched for this.

@kjin
Copy link
Contributor Author

kjin commented Oct 14, 2017

@ofrobots Probably because it's unmergeable... rebased

@DominicKramer
Copy link

LGTM: provided the tests pass

Copy link
Contributor

@ofrobots ofrobots left a comment

Choose a reason for hiding this comment

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

LGTM if the coverage impact is positive.

@codecov-io
Copy link

codecov-io commented Oct 17, 2017

Codecov Report

Merging #62 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #62      +/-   ##
==========================================
+ Coverage   99.58%   99.59%   +0.01%     
==========================================
  Files           8        8              
  Lines         241      249       +8     
  Branches        8       10       +2     
==========================================
+ Hits          240      248       +8     
  Misses          1        1
Impacted Files Coverage Δ
test/test-kitchen.ts 100% <100%> (ø) ⬆️
src/clean.ts 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f8a049...dbb6630. Read the comment docs.

@ofrobots
Copy link
Contributor

When landing, make sure the commit includes the 'Fixes: #60' metadata.

@kjin
Copy link
Contributor Author

kjin commented Oct 19, 2017

Since the approvals were issued, I added a test... was able to get code coverage to increase by 0.01%

A point of possible contention is here. The explanation is that exec shouldn't actually be promisified the way it is because the callback may populate err and stdout/stderr. So I added a new function to always resolve with an exit code (though most places still use the now-renamed "simple" execp)

Also, two things happened:

  • clang-format updated, meaning things had to be re-formatted
  • chalk got built-in TS definitions that clash with @types/chalk, so I removed the dependency on the latter (2e4711a)

@DominicKramer @ofrobots PTAL at the two linked things.

const renamep = pify(fs.rename);
const ncpp = pify(ncp.ncp);

const execp =
Copy link
Contributor

Choose a reason for hiding this comment

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

This deserves a comment. Also, we should probably look for an ecosystem module for execp.

@ofrobots
Copy link
Contributor

Rule of thumb: if you have explain why some code is the way it is in the PR, you probably also need it as a comment.

LGTM once that is addressed.

@kjin kjin merged commit d8789a4 into google:master Oct 19, 2017
@kjin kjin mentioned this pull request Jun 26, 2018
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants