Closed MRdNk closed 11 years ago
There is a double callback on XML parsing error which I would like to fix before releasing... would it be OK for you to wait for like a week or so?
Sorry, but with that bug in, I kind of feel bad to go out for a new release...
On Mon, Jun 3, 2013 at 2:32 PM, Duncan Angus Wilkie < notifications@github.com> wrote:
Can we get this pushed to npm?
I'm using these bug fixes in a project.
You can merge this Pull Request by running
git pull https://github.com/MRdNk/node-pivotal master
Or view, comment on, or merge it at:
https://github.com/Wizcorp/node-pivotal/pull/14 Commit Summary
- Patch version bump to 0.1.4
File Changes
- M package.jsonhttps://github.com/Wizcorp/node-pivotal/pull/14/files#diff-0(2)
Patch Links:
http://www.wizcorp.jp/Marc Trudel-Belisle
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
Sure. Have you figured out where the bug is? Or have a use case that's causing it? Maybe I can help.
I have to look deeper into it, but basically empty lists of anything would normally double callback for some reason (one cb with an error, another without). This seems to do with the xml2js library, which can call a call back and throw an error (from what I have seen, for some reason the cb is called before the error is thrown)...
If you have some time, try to use the test script and specify manually a project id where you have no stories, and you should see the problem occur.
After some investigation, I think this is more to do with the try, catch outside of the callback, take a look at https://github.com/MRdNk/node-pivotal/commit/492139b4ec5696ae414b123e036f8a93e6fa9952
Calling getStories 840091
callback: { '0': null, '1': {} }
Got project's stories! {}
Unless I'm being naive?
It may very well be the case, but that would mean that the xml2js parsing process is synchronous?
As in, what would be happening with the test is:
1) make a call 2) callback 3) exception/error is thrown in the callback function 4) try/catch catches it 5) calls the same callback, again
Merh. I'll take a look at xml2js' code quickly, but I get the feeling this is what is happening.
On Wed, Jun 5, 2013 at 5:22 AM, Duncan Angus Wilkie < notifications@github.com> wrote:
Unless I'm being naive?
— Reply to this email directly or view it on GitHubhttps://github.com/Wizcorp/node-pivotal/pull/14#issuecomment-18936370 .
http://www.wizcorp.jp/Marc Trudel-Belisle
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
I've just re-added the try, catch - and the problem, no longer exists, I think this was actually a false negative. The tests were throwing the error -
ret.story.length
https://github.com/MRdNk/node-pivotal/commit/492139b4ec5696ae414b123e036f8a93e6fa9952#L1L353
I don't think the try catch should be there, as an error like this should kill the program.
The next question that pops up in my mind, is around how the data should be returned to the module consumer with a project with:
At the moment it works like this
I think there should be consistancy, all should return an array in story, just with .length 0, 1, 1+
What are your thoughts on this?
Yeah, that is what I mean. In fact, as I was describing, the try catch will trigger if you have an exception case in your callback, which will mean you will call back your callback again.
On Wed, Jun 5, 2013 at 3:33 PM, Duncan Angus Wilkie < notifications@github.com> wrote:
I've just re-added the try, catch - and the problem, no longer exists, I think this was actually a false negative. The tests were throwing the error -
ret.story.length
- cannot get length of undefined.
MRdNk@492139b#L1L353https://github.com/MRdNk/node-pivotal/commit/492139b4ec5696ae414b123e036f8a93e6fa9952#L1L353
I don't think the try catch should be there, as an error like this should kill the program.
— Reply to this email directly or view it on GitHubhttps://github.com/Wizcorp/node-pivotal/pull/14#issuecomment-18957982 .
http://www.wizcorp.jp/Marc Trudel-Belisle
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
Absolutely agree.
On Wed, Jun 5, 2013 at 3:58 PM, Duncan Angus Wilkie < notifications@github.com> wrote:
The next question that pops up in my mind, is around how the data should be returned to the module consumer with a project with:
- no stories
- one story
- more than one story
At the moment it works like this
- no stories == {}
- one story == {story: {storyObject}
- more than one story == {story: [{storyObject}, {storyObject}]}
I think there should be consistancy, all should return an array in story, just with .length 0, 1, 1+
What are your thoughts on this?
— Reply to this email directly or view it on GitHubhttps://github.com/Wizcorp/node-pivotal/pull/14#issuecomment-18958743 .
http://www.wizcorp.jp/Marc Trudel-Belisle
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
Cool, well I'm at my groups hacknight tonight, so I might try and fix this then.
Nice! Thanks!
On Wed, Jun 5, 2013 at 4:07 PM, Duncan Angus Wilkie < notifications@github.com> wrote:
Cool, well I'm at my groups hacknight tonight, so I might try and fix this then.
— Reply to this email directly or view it on GitHubhttps://github.com/Wizcorp/node-pivotal/pull/14#issuecomment-18959006 .
http://www.wizcorp.jp/Marc Trudel-Belisle
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
Closed for cleaner commits.
Can we get this pushed to npm? I'm using these bug fixes in a project.