Skip to content
This repository has been archived by the owner on Feb 20, 2019. It is now read-only.

WIP: Cannot get coverage quite there #9

Merged
merged 1 commit into from
Feb 3, 2016
Merged

WIP: Cannot get coverage quite there #9

merged 1 commit into from
Feb 3, 2016

Conversation

kentcdodds
Copy link
Member

For some reason I was unable to get code coverage to 100% could you please help and provide any other feedback? Thanks!

Here's the coverage report:

screen shot 2016-01-30 at 10 46 46 pm

It looks like I'm just missing coverage for the case where there is no padWith specified. Should I just make a test that covers that case?

@codecov-io
Copy link

Current coverage is 100.00%

Merging #9 into master will not affect coverage as of c1b8b3f

@@            master      #9   diff @@
======================================
  Files            3       4     +1
  Stmts           12      16     +4
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit             12      16     +4
  Partial          0       0       
  Missed           0       0       

Review entire Coverage Diff as of c1b8b3f

Powered by Codecov. Updated on successful CI builds.

*/
function padLeft(str, size, padWith) {
if (size <= str.length) {
return str
Copy link
Collaborator

Choose a reason for hiding this comment

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

To cover this code, simply add a test that calls padLeft with a size that's smaller than the length of the string you give. For example: padLeft('12345', 3) should do the trick. 👍

@silvestertomato
Copy link
Collaborator

This looks great! Please add that test. Let me know if you need more help! Thanks!

@kentcdodds
Copy link
Member Author

I had a little trouble with the commit message, so I ran my commit with --no-verify. Could you help me get that right please? Thanks!

@silvestertomato
Copy link
Collaborator

Sure thing! The CONTRIBUTING.md explains our convention a bit. And if you'd like you can add you changes with git add . and then run npm run commit and you'll be given an interactive prompt to use for creating your commit message.

@silvestertomato
Copy link
Collaborator

It looks like there have been changes in the master branch since you made your changes and there's a merge conflict. Could you kindly rebase your branch with master and resolve any conflicts? Let me know if you need any help doing that!

@silvestertomato
Copy link
Collaborator

Awesome job on the rebase! This is ready to be merged. One more thing though, could you please squash these commits into a single commit with a message that follows our commit message conventions?

@kentcdodds
Copy link
Member Author

It's ready to go!

@silvestertomato
Copy link
Collaborator

Fantastic work. This is perfect. I'll merge this pull request and the module will be automatically released using semantic-release (find out how here). Thanks @kentcdodds! 🎊 🎉 :shipit: 🚀

silvestertomato added a commit that referenced this pull request Feb 3, 2016
WIP: Cannot get coverage quite there
@silvestertomato silvestertomato merged commit 65c9793 into eggheadio-github:master Feb 3, 2016
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.

None yet

3 participants