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

rewrote the express example using the 2.0.0 driver #7

Merged
merged 1 commit into from
Apr 29, 2015
Merged

rewrote the express example using the 2.0.0 driver #7

merged 1 commit into from
Apr 29, 2015

Conversation

Prinzhorn
Copy link
Contributor

See #6

I left the angular code untouched, except for updating the URLs and fixing space/quote inconsistencies.

The app.js is basically completely rewritten to use the new driver (and a single connection), correctly handle errors and use the async module for readability.

This is the first time I have ever used RethinkDB, so please comment on all the things. I don't know if I've followed best practices etc.

After we're happy with it, we need someone for the koa and promises rewrite as I don't have any experience with them and don't know about best practices.

Ping @deontologician and @robertklep for review

@Prinzhorn Prinzhorn mentioned this pull request Apr 28, 2015
@Prinzhorn
Copy link
Contributor Author

Any thoughts on code style? I went with my personal preference (tabs) since the project was inconsistent with tabs and four spaces. I also went with single quotes as the project had mixed single and double quotes.

@robertklep
Copy link

LGTM 👍 (also tested it locally, it's running fine)

I don't care much for tabs (2-space indents #ftw), but mixing is bad so either tabs or spaces for consistency. Same for quotes.

};

$scope.clearCompletedTodos = function () {
$scope.todos = todos.filter(function (val) {
$scope.todos = $scope.todos.filter(function (val) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should call DELETE /todos/:id for all completed ids. Right now it results in weird behavior where "cleared" completed todos can come back

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've noticed this as well, but I know nothing about angular (changing this line did at least get rid of the exception because todos was undefined). I'll have a look at it.

@deontologician deontologician self-assigned this Apr 28, 2015
@deontologician
Copy link
Contributor

Looks good, I really like async.waterfall, that's a nice library.

Other than the issue I noted about clearing completed items, it looks good. I also agree with @robertklep that 2 spaces is better than tabs, but I wouldn't not accept the PR over that

@Prinzhorn
Copy link
Contributor Author

I'll look into the issue and change the indentation later today.

@Prinzhorn
Copy link
Contributor Author

2 spaces, single quotes and fixed the "clear completed" thing.

deontologician added a commit that referenced this pull request Apr 29, 2015
rewrote the express example using the 2.0.0 driver
@deontologician deontologician merged commit 311bfd4 into rethinkdb:master Apr 29, 2015
@deontologician
Copy link
Contributor

Looks good. Thanks a lot @Prinzhorn

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

3 participants