conventional-changelog-archived-repos / validate-commit-msg

DEPRECATED. Use https://github.com/marionebl/commitlint instead. githook to validate commit messages are up to standard
http://conventionalcommits.org/
MIT License
556 stars 101 forks source link

refactor(index): use ES2015 #39

Closed joaocunha closed 7 years ago

joaocunha commented 7 years ago
kentcdodds commented 7 years ago

Cool! I'm not certain that I'm ready to drop support for older versions of node (so if we want to do this, we should probably transpile with babel). Would you be willing to do that?

kentcdodds commented 7 years ago

Oh, also, we should probably update the node version that travis is running on. I don't care about iojs versions anymore. We should probably test this on 0.12.0, 4.x, and 6.x

joaocunha commented 7 years ago

@kentcdodds I've iterated over your suggestions on https://github.com/kentcdodds/validate-commit-msg/pull/39/commits/f11e82c51aeb3e95715faec747190067d334889a.

It's the first time I'm using Babel on a node lib (I usually use it on client scripts only) so feedback is appreciated.

codecov-io commented 7 years ago

Current coverage is 100% (diff: 100%)

Merging #39 into master will not change coverage

@@           master   #39   diff @@
===================================
  Files           1     1          
  Lines          88    87     -1   
  Methods         0     0          
  Messages        0     0          
  Branches        0     0          
===================================
- Hits           88    87     -1   
  Misses          0     0          
  Partials        0     0          

Powered by Codecov. Last update d8a2d19...4d0e7c2

kentcdodds commented 7 years ago

Cool! I'll make a few notes. It might help communicate what I'm looking for better if you check out this and this

kentcdodds commented 7 years ago

For the most part it looks good. Thanks for giving this some work!

joaocunha commented 7 years ago

@kentcdodds: fixed based on your comments. Thanks for the feedback!

There's two things I wonder, tho:

  1. I've chosen the install script to run the build as the ./build directory is not being tracked. On a fresh npm install validate-commit-msg, wouldn't the ES5 file be missing?
  2. Tests are now failing on v0.12.0 because it can't parse ES2015 (we're now testing the source, not the build). How should I tackle that?
kentcdodds commented 7 years ago

Did you take a look at the videos I mentioned? They should answer these questions :)

joaocunha commented 7 years ago

@kentcdodds that was invaluable. In fact, I was planning to watch your series a while ago :)

I've fixed it following your videos, but I still can't get how the build process will be triggered once someone installs the module. I might have missed the point where you talk about it (I saw the Travis bit, tho).

joaocunha commented 7 years ago

Travis is complaining:

ERROR: Coverage for branches (83.33%) does not meet global threshold (90%)

But it's fine (> 90%) on my local:

image

I'll investigate on this one later.

kentcdodds commented 7 years ago

Actually, I think the issue you're seeing is that version 7 of nyc changed how you configure it to cover ES6 tests. You can see this for an updated example (here are some docs.

Thanks again for working on this!

kentcdodds commented 7 years ago

Things have changed quite a bit. I think we'll go ahead and close this now.