search

Wizcorp / node-pivotal

NodeJS API library for PivotalTracker
44 stars 20 forks source link

Get stories always arrays #19

Closed MRdNk closed 11 years ago

MRdNk commented 11 years ago

Tidier commits.

No story

callback: { '0': null, '1': { story: [] } }

1 Story

callback: { '0': null, '1': { story: [ [Object] ] } }

2 Stories

callback: { '0': null, '1': { story: [ [Object], [Object] ] } }
MRdNk commented 11 years ago

"... it would be more convenient to return the array directly instead of the object." @stelcheck - I agree, but this would be quite a big change to the api, so perhaps should be another minor patch.

stelcheck commented 11 years ago

Fair enough. I agree this is not a changed required for this release.

I'll check the code and come back to you.

stelcheck commented 11 years ago

Outside of the version bump, looks pretty good. I think this is something we will want to apply pretty much everywhere in the API: returning array of values by default would just make everyone's lives easier.

Once we are fixed on the version bump, I'll make sure to merge and do a release.

stelcheck commented 11 years ago

By the way, I will fix the double callback issue myself. Basically, the best to do here is to remove the try-catch, and put the callback in process.nextTick (which will help generate a more localized and more meaningful error output when the user's code crashes)

MRdNk commented 11 years ago

Removing the try catch is the way to go. - This makes sense to be a patch bump - though could break someones code.

MRdNk commented 11 years ago

getStories Minor Version bump: The potential issue, is that you break someone's project; for example I noticed this issue and fixed with by checking if there is an empty data value as no stories, and a single data.story object as one story else accept an array of data.story.

So that's where my head was, when I thought about the version bump to 0.2.0. A few people use "node-pivotal": "0.1.x" in a package.json dependencies.

But I agree that it's not an easy decision.

stelcheck commented 11 years ago

The try catch was introduced in between releases, so it shouldnt break anyone's code. However, the process.nextTick wrap does make life easier for debugging, as code in your own code (e.g. in your callback) will not bubble up and make xml2js throw an error, which in turns returns a very nasty, unreadable stack trace.

As for release version, you are right that code behavior on the user's end will be different. Let's keep it to 0.2.0.

Ill do a quick 0.1 release, then merge and release this branch.

One last thing: you have been contributing a lot, feel free to add yourself as a contributor in the package.json before I merge :)

MRdNk commented 11 years ago

"One last thing: you have been contributing a lot, feel free to add yourself as a contributor in the package.json before I merge :)"

stelcheck commented 11 years ago

Merh. Sorry, my version bump made your PR unmergable. Ill manually merge, then.

stelcheck commented 11 years ago

And, released! Thanks for your help!

MRdNk commented 11 years ago

Excellent, thank you. When I get a chance, I'll look at changing it to simply an array and looking at the other API calls.

The xml attribute 'type=boolean' has now moved to an open issue on pivotal tracker support.

stelcheck commented 11 years ago

Good news!

I will open new issues for the things I think should be improved. Always returning array should probably be one, but I would also like to add a command-line shell or app to interacting with PT. This would make it easier for people to, say, have local git hooks or command to operate.

Again, thanks for the help!

On Fri, Jun 7, 2013 at 4:46 PM, Duncan Angus Wilkie < notifications@github.com> wrote:

Excellent, thank you. When I get a chance, I'll look at changing it to simply an array and looking at the other API calls.

The xml attribute 'type=boolean' has now moved to an open issue on pivotal tracker support.

— Reply to this email directly or view it on GitHubhttps://github.com/Wizcorp/node-pivotal/pull/19#issuecomment-19093413 .

http://www.wizcorp.jp/Marc Trudel-Belisle

Chief Technology Officer | Wizcorp Inc. http://www.wizcorp.jp/

TECH . GAMING . OPEN-SOURCE WIZARDS+ 81 3-4550-1448|Websitehttp://www.wizcorp.jp/ |Twitter https://twitter.com/Wizcorp|Facebookhttp://www.facebook.com/Wizcorp |LinkedIn http://www.linkedin.com/company/wizcorp

MRdNk commented 11 years ago

command-line shell is a great idea - I was thinking about something like this for my own project, that uses this module.

Basically my project, allows you to add a new project, which creates a db entry, adds a pt project and a github repo with a single form, and a repl / cmd would be a pretty good alternative for cmd line people.

And thanks for the work on this project and taking my pull requests & help.

stelcheck commented 11 years ago

Created an issue to work on this: https://github.com/Wizcorp/node-pivotal/issues/21

Will keep it updated as I get some new ideas, but of course I hope others will pitch in :D

On Fri, Jun 7, 2013 at 4:54 PM, Duncan Angus Wilkie < notifications@github.com> wrote:

command-line shell is a great idea - I was thinking about something like this for my own project, that uses this module.

Basically my project, allows you to add a new project, which creates a db entry, adds a pt project and a github repo with a single form, and a repl / cmd would be a pretty good alternative for cmd line people.

— Reply to this email directly or view it on GitHubhttps://github.com/Wizcorp/node-pivotal/pull/19#issuecomment-19093646 .

http://www.wizcorp.jp/Marc Trudel-Belisle

Chief Technology Officer | Wizcorp Inc. http://www.wizcorp.jp/

TECH . GAMING . OPEN-SOURCE WIZARDS+ 81 3-4550-1448|Websitehttp://www.wizcorp.jp/ |Twitter https://twitter.com/Wizcorp|Facebookhttp://www.facebook.com/Wizcorp |LinkedIn http://www.linkedin.com/company/wizcorp