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

create file getAllRoutes and add tests #47

Merged
merged 12 commits into from
Nov 18, 2016
Merged

Conversation

wcastand
Copy link
Contributor

@wcastand wcastand commented Oct 21, 2016

A first try, what do you think about this implementation ?

I was thinking maybe add an object instead of a string to accès path and cb.
And maybe an object or array with the param keys.

@yoshuawuyts
Copy link
Member

Sweet, nice work! I think an Object might indeed be valuable, as it allows for some interesting tricks. People can then get all routes from the object by using Object.keys().

Small comment here though: as this is a low level package I'm cool if you use some ES6 in the tests, but none (except for const) in the actual implementation. This is due to a variety of reasons, amongst others due to compilation speed, legacy support and performance - but primarily for consistency with all the other packages I've written. Thanks for understanding! ✨

@wcastand
Copy link
Contributor Author

I update the PR, i'm not sure i did what you wanted so let me know if it's not what you had in mind :)
getAllRoutes returns an Object like this:

{
  '/foo': function(x, y) { return x * y },
  '/foo/bar': function(x, y) { return x / y},
  '/bar/bchi': function(x, y) { return x%y}
}

i think i delete all the es6 stuff, but since i didn't write es5 for quite some time i'm not sure about what is es5 and es6 anymore :/

@coveralls
Copy link

coveralls commented Oct 24, 2016

Coverage Status

Coverage decreased (-81.8%) to 14.141% when pulling 075f3d7 on wcastand:master into 9241bd1 on yoshuawuyts:master.

@coveralls
Copy link

coveralls commented Oct 24, 2016

Coverage Status

Coverage decreased (-43.02%) to 52.941% when pulling 075f3d7 on wcastand:master into 9241bd1 on yoshuawuyts:master.

@coveralls
Copy link

coveralls commented Oct 24, 2016

Coverage Status

Coverage decreased (-43.02%) to 52.941% when pulling a777e96 on wcastand:master into 9241bd1 on yoshuawuyts:master.

@coveralls
Copy link

coveralls commented Oct 24, 2016

Coverage Status

Coverage decreased (-43.02%) to 52.941% when pulling a777e96 on wcastand:master into 9241bd1 on yoshuawuyts:master.

@coveralls
Copy link

coveralls commented Oct 24, 2016

Coverage Status

Coverage decreased (-43.02%) to 52.941% when pulling a777e96 on wcastand:master into 9241bd1 on yoshuawuyts:master.

@coveralls
Copy link

coveralls commented Oct 24, 2016

Coverage Status

Coverage decreased (-38.1%) to 57.851% when pulling ddac19b on wcastand:master into 9241bd1 on yoshuawuyts:master.

@coveralls
Copy link

coveralls commented Oct 24, 2016

Coverage Status

Coverage decreased (-38.1%) to 57.851% when pulling ddac19b on wcastand:master into 9241bd1 on yoshuawuyts:master.

@wcastand
Copy link
Contributor Author

I have no idea why is only testing my test file.
Help welcome on this :)

@yoshuawuyts
Copy link
Member

odd, not sure why it's failing - perhaps try updating deps, running node@4 and node@6 in .travis.yml hmmmm -if that fails try running it through $ tap tests/*

@yoshuawuyts
Copy link
Member

implementation is looking good tho (:

@wcastand
Copy link
Contributor Author

wcastand commented Oct 25, 2016

In local when i try npm run test. It does all the tests.
i update packages, let's see.

It's still works on local.

@coveralls
Copy link

Coverage Status

Coverage decreased (-38.1%) to 57.851% when pulling f9e6591 on wcastand:master into 9241bd1 on yoshuawuyts:master.

1 similar comment
@coveralls
Copy link

coveralls commented Oct 25, 2016

Coverage Status

Coverage decreased (-38.1%) to 57.851% when pulling f9e6591 on wcastand:master into 9241bd1 on yoshuawuyts:master.

@yoshuawuyts
Copy link
Member

hah, yeah it also works on my machine haha

On Tue, Oct 25, 2016 at 4:57 PM Coveralls notifications@github.com wrote:

[image: Coverage Status] https://coveralls.io/builds/8499705

Coverage decreased (-38.1%) to 57.851% when pulling f9e6591
f9e6591
on wcastand:master
into 9241bd1
9241bd1
on yoshuawuyts:master
.


You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#47 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACWleooz59YW_ODWh3Q4MaxyzSoABEo5ks5q3hjegaJpZM4KdpUG
.

@alexandrebodin
Copy link

alexandrebodin commented Oct 26, 2016

Hi guys.

After looking at your previous builds your cover script is at fault.

Running instabul cover test/* is equivalent to running node test/* with cover which only runs one test file.

Until now all your coverage was only running the test router.js

To fix it you only need to do this in your package.json

"test-cov": "NODE_ENV=test istanbul cover tape -- test/*"

Hope it helped ;)

Cheers

@coveralls
Copy link

coveralls commented Oct 26, 2016

Coverage Status

Coverage decreased (-81.08%) to 14.876% when pulling 8fee143 on wcastand:master into 9241bd1 on yoshuawuyts:master.

@wcastand
Copy link
Contributor Author

es6 in test for walk.js break travis. I will correct them tomorrow

@coveralls
Copy link

Coverage Status

Coverage increased (+1.1%) to 97.08% when pulling c5e69cc on wcastand:master into 9241bd1 on yoshuawuyts:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+1.1%) to 97.08% when pulling c5e69cc on wcastand:master into 9241bd1 on yoshuawuyts:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.1%) to 97.08% when pulling c5e69cc on wcastand:master into 9241bd1 on yoshuawuyts:master.

@yoshuawuyts
Copy link
Member

Niceee - is this ready for review now?

On Thu, Oct 27, 2016, 10:08 AM Coveralls notifications@github.com wrote:

[image: Coverage Status] https://coveralls.io/builds/8534635

Coverage increased (+1.1%) to 97.08% when pulling c5e69cc
c5e69cc
on wcastand:master
into 9241bd1
9241bd1
on yoshuawuyts:master
.


You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#47 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACWletVUqmIxJ7qsMpffORAtjzWlHEOGks5q4FvngaJpZM4KdpUG
.

@wcastand
Copy link
Contributor Author

wcastand commented Oct 27, 2016

It is :)

@wcastand
Copy link
Contributor Author

wcastand commented Nov 3, 2016

Do i need to do something here. Or you just didn't have the time to look at it ?
i'm just asking in case i missed something

@yoshuawuyts
Copy link
Member

Thanks so much for this! - sorry for not merging earlier; been a bit low on bandwidth lately ✨

@yoshuawuyts yoshuawuyts merged commit 62639f2 into choojs:master Nov 18, 2016
@yoshuawuyts
Copy link
Member

v6.3.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants