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

feat(max): Add max function (#114) #115

Merged
merged 3 commits into from
Aug 21, 2017

Conversation

mis101bird
Copy link
Contributor

the max function could find the maximum value of the list
Please review the implementation
Thanks :)

Closes #114

@codecov-io
Copy link

codecov-io commented Aug 20, 2017

Codecov Report

Merging #115 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #115   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          40     41    +1     
  Lines         109    113    +4     
=====================================
+ Hits          109    113    +4
Impacted Files Coverage Δ
src/max.js 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 5dfec97...31c9877. Read the comment docs.

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks! Just one minor change :)

test/max.test.js Outdated
@@ -0,0 +1,8 @@
import test from 'ava'
import max from '../src/max'
Copy link
Member

Choose a reason for hiding this comment

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

This should be: import {max} from '../src' (see the other tests).
This means that you'll need to import max into the src/index.js and add it to the list of exports. Could you do that update please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK! no problem

the max function could find the maximum value of the list

Closes eggheadio-github#114
@mis101bird
Copy link
Contributor Author

I don't know why I get the error after I revise the test
failed with "(0 , _src.max) is not a function"

@kentcdodds
Copy link
Member

Go ahead and push what you have and I can help 👍

@mis101bird
Copy link
Contributor Author

I already push the project as the same commit. please review the commit 31c9877
Sorry I rebase the commit before I found the error 😭 Because I didn't expect the error.

@@ -0,0 +1,8 @@
import test from 'ava'
import {max} from '../src'
Copy link
Member

Choose a reason for hiding this comment

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

Ah, the reason you're having this problem is because you haven't added the import/export to src/index.js yet. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh! Got it! I done XD
Does import file by this way could have better performance? :)

Copy link
Member

Choose a reason for hiding this comment

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

No, but it makes it easier for folks to use this package:

import {max} from 'stack-overflow-copy-paste'

Even though nobody should really be using this package 😉 It's just for folks to practice contributing to OSS!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it 👍

@kentcdodds kentcdodds merged commit 3c21883 into eggheadio-github:master Aug 21, 2017
@kentcdodds
Copy link
Member

Thanks so much! You did great! 👏

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants