RIAEvangelist / node-dominos-pizza-api

This is a node.js wrapper for the dominos pizza apis
MIT License
538 stars 129 forks source link

Newest pull breaks example/api-examples #8

Closed RIAEvangelist closed 9 years ago

RIAEvangelist commented 9 years ago

If anyone gets time please assign to self and commit fixes

:~/git/node-dominos-pizza-api/example/api-examples$ node dominos-store-data.js 

module.js:338 throw err; ^ Error: Cannot find module './src/Order' at Function.Module._resolveFilename (module.js:336:15) at Function.Module._load (module.js:278:25) at Module.require (module.js:365:17) at require (module.js:384:17) at Object. (/home/bmiller/git/node-dominos-pizza-api/dominos-pizza-api.js:14:12) at Module._compile (module.js:460:26) at Object.Module._extensions..js (module.js:478:10) at Module.load (module.js:355:32) at Function.Module._load (module.js:310:12) at Module.require (module.js:365:17)

madelinecameron commented 9 years ago

This is a bit more broken than I expected. I found a few more issues that need to be fixed to get this working and then I will write more tests for everything because what I am finding is discouraging.

The first issue is that Git didn't pick up on me renaming 'order.js' to 'Order.js' which caused this issue in the first place.

The second issue that findNearbyStores doesn't work:

  1. With a ZIP code without funky formatting (['', '63102'] is what has to be passed) and I assume this goes from city + zip as well. Basically, anything but an address is broken currently.
  2. Without an array being passed (Expected should be string). This isn't totally bad, it is just unintuitive. This could be fixed by breaking the "address string" into an array inside the function and this could solve the first issue.

I will work on this more after I get home tonight.

RIAEvangelist commented 9 years ago

Awesome, another fix would be allow either an array or a string to be passed and use defauilts Like :

RIAEvangelist commented 9 years ago

apparently ctrl+enter submits the comment... (meant to hit shit+enter)

function findNearbyStores(address){
    if(typeof address=='string'){
        address==['',address]
    }

   ...

}

This is how a lot of frameworks work. They allow for the simplest implementation for devs, but also more complex calls for advanced users.

RIAEvangelist commented 9 years ago

Address.parse

Looks like the issue arises from not using the Address.parse call on the passed address.

This would probably fix everything.

madelinecameron commented 9 years ago

That's true, that'd be best. I can add that functionality as well.

I'll write tests to see how much of my code is broken. There were things I left broken / didn't test by hand because PizzaGiver didn't use the functionality! D:

I will add the functionality to allow arrays as well. My current commit doesn't but I'll fix it once I get home (I have to run out to take a test).

madelinecameron commented 9 years ago

Yes, Address.parse would likely work on that as well. The only issue is that Address.parse splits by comma and findNearbyStores only has room for two lines.

I believe Domino's end expects [Address, City-State-Zip] which could be easily solved by using Address.parse and then joining the last three indexes?

RIAEvangelist commented 9 years ago

Do you mind if I reassign to me?

This would be a good way for me to get familiar with some stuff.

This should actually probably use the Address Object which will automatically parse the string.

madelinecameron commented 9 years ago

Nope, not at all. I assigned to myself because i was the one who broke it, haha. :)

RIAEvangelist commented 9 years ago

Lol teamwork

I got it working now and understand things better now.

I'm gonna comment out stripe like we talked in #13, and look into how their api works for some examples and documentation.

Should we start a gutter chat for the repo?