Skip to content
This repository has been archived by the owner on Jun 22, 2022. It is now read-only.

Code improvements #12

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Code improvements #12

wants to merge 4 commits into from

Conversation

aderyabin
Copy link
Contributor

This PR hasn't big functionality changes. Only code style improvements and fix messages.

@danielmewes
Copy link
Member

Thanks a lot for your pull requests @aderyabin!
Unless you have already done so, can you please sign our contributor agreement http://rethinkdb.com/community/cla/ ?

@mlucy Can you take a look?

@aderyabin
Copy link
Contributor Author

@danielmewes Done!

@mlucy
Copy link
Member

mlucy commented Dec 20, 2014

Sorry I haven't gotten a chance to look at this yet @aderyabin . The holidays are making my schedule pretty hectic, but I'll try to get to it soon.

@mlucy
Copy link
Member

mlucy commented Jan 14, 2015

Alright, I'm back from vacation and finally got to this!

The changes look good, except for the changes like r.table('test') to r.table(:test). The former reads much better to me. 'test' isn't the name of a field in a hash, it's just the name of something, and it's a little weird to have it be a symbol in that context. I'm torn on whether or not to make the arguments to e.g. pluck be symbols or strings, since they are field names, but I'm leaning toward strings there too just for consistency and because it prevents people from making errors (e.g. r.table('test').get(0)[:foo].run() works but r.table('test').get(0).run()[:foo] doesn't (but r.table('test').get(0).run()['foo'] does)).

If the symbols used in queries change to strings, I'm on board with these changes.

@danielmewes -- who wrote this code originally? They should probably double-check this too.

@danielmewes
Copy link
Member

@mlucy It seems @al3xandru did, so that might be difficult. @chipotle and @mglukhovsky have both done smaller maintenance since then https://github.com/rethinkdb/rethinkdb-example-sinatra-pastie/commits/master .

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants