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

fix errors being triggered on successful requests #22

Merged
merged 1 commit into from Mar 26, 2015

Conversation

Eluinhost
Copy link
Contributor

The error id is a number not a string, stops errors being triggered incorrectly

The error id is a number not a string, stops errors being triggered incorrectly
@zyberspace
Copy link
Contributor

I actually changed it to a string here: 25ad30f. Which version is right now? :o

@Eluinhost
Copy link
Contributor Author

First time I've looked at the project but I think it has to do with this change later on which then parsed it as an number: e371174

@zyberspace
Copy link
Contributor

but I think it has to do with this change later on which then parsed it as an number: e371174

Oh yeah, that looks totally right, so ty then :D
That's why micro-optimization is bad XD (36b99fb)

timklge pushed a commit that referenced this pull request Mar 26, 2015
Fix errors being triggered on successful requests
@timklge timklge merged commit 95f49a1 into timklge:master Mar 26, 2015
@timklge
Copy link
Owner

timklge commented Mar 26, 2015

Unfortunately I can't test on my own atm. Thanks for the fix!

timklge pushed a commit that referenced this pull request Mar 26, 2015
Bugfix release, #22
@hnrch02
Copy link

hnrch02 commented May 15, 2015

First of all, this was a breaking change introduced in a patch version (I'm assuming this project follows SemVer?) but secondly and more importantly how am I supposed to know now whether actions like "login" or "use" were successful when all three arguments supplied to the callback (error, response, rawResponse) are all undefined?

@zyberspace
Copy link
Contributor

@hnrch02: Why is this a breaking change? It is only backend error parsing.
Also it is a fix, so patch version is completely fine.

@hnrch02
Copy link

hnrch02 commented May 16, 2015

Yes, it is a backend error parsing change which removes the { id: 0, msg: 'ok' } "error" object which I was using the identify whether login and use were successfully executed. It broke my application when updating to the latest version of this package thus being a breaking change.

Anyway, it makes more sense that error is undefined when there is actually no error so I'll just change my app to adapt to this change. I was just surprised by this which I again did not expect to see in a patch version.

@zyberspace
Copy link
Contributor

OK, i am sry then for this, but this behaviour (undefined when no error) was always wanted this way.
So this is still just a bug-fix, but i get your point.

If i have time, i will maybe take a deeper look over the whole code and, additionally, create some tests. Those should prevent stuff like this then (in other words: yes, you are completely right! ;) ).

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.

None yet

4 participants