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

(#1167) - acutal error objects #1168

Merged
merged 1 commit into from Dec 21, 2013
Merged

(#1167) - acutal error objects #1168

merged 1 commit into from Dec 21, 2013

Conversation

calvinmetcalf
Copy link
Member

pull for #1167, tests inexplicably fail in node for me, not sure why.

@calvinmetcalf
Copy link
Member Author

nope not just me

@calvinmetcalf
Copy link
Member Author

now with passing tests!

return obj;
}
});
}
call(cb, null, obj, resp);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the spacing here is all wrong (are we linting this?), also the use of obj is very confusing

@daleharvey
Copy link
Member

There is some nits around inconsistent spacing

if (stuff) { 
    ... 
} else { 

etc, I though we were enforcing that via jshint, but dont have time to check right now sorry, otherwise looks great 👍 to merge with nits fixed

@calvinmetcalf
Copy link
Member Author

hm lemme look

@calvinmetcalf
Copy link
Member Author

ah we forgot to add deps to the jshint path

@calvinmetcalf
Copy link
Member Author

on second thought I'm going to fix the lint errors that apply to this pull and not try to get all of deps to pass jshint as that turned out to be a more massive undertaking

@daleharvey
Copy link
Member

Originally 'deps' were external deps and specifically not jshinted (stuff
like jquery), but at this point very little of it isnt ours, should have
thought about before starting to merge code in there, sorry

On 21 December 2013 17:31, Calvin Metcalf notifications@github.com wrote:

on second thought I'm going to fix the lint errors that apply to this pull
and not try to get all of deps to pass jshint as that turned out to be a
more massive undertaking


Reply to this email directly or view it on GitHubhttps://github.com/daleharvey/pouchdb/pull/1168#issuecomment-31067800
.

@daleharvey
Copy link
Member

And also +1 on merging this prior to doing a silly large jshint commit, (another nit would be spelling error in commit message)

Sorry I am not merging stuff at the moment, on a boat with crappy wifi

@calvinmetcalf calvinmetcalf merged commit 702e79f into master Dec 21, 2013
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

2 participants