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

Jump to bottom

Properly quote JS object keys in toString() #4997

Merged
merged 2 commits into from Oct 27, 2015
Merged

Properly quote JS object keys in toString() #4997

merged 2 commits into from Oct 27, 2015

Conversation

mikemintz
Copy link
Contributor

This addresses part of issue #2372.

Before this change:

r.table('turtles', {readMode: 'single'}).toString()
'r.table("turtles", {readMode: "single"})'

r.expr({'-x-y-z': 2}).merge({b: [1,2]}).toString()
'r({-x-y-z: "b"}).merge({b: [1, 2]})'

After this change:

r.table('turtles', {readMode: 'single'}).toString()
'r.table("turtles", {"readMode": "single"})'

r.expr({'-x-y-z': 2}).merge({b: [1,2]}).toString()
'r({"-x-y-z": "b"}).merge({"b": [1, 2]})'

@mikemintz
Properly quote JS object keys in toString()
d63afdb 
This addresses part of issue rethinkdb#2372.

Before this change:

  > r.table('turtles', {readMode: 'single'}).toString()
  'r.table("turtles", {readMode: "single"})'

  > r.expr({'-x-y-z': 2}).merge({b: [1,2]}).toString()
  'r({-x-y-z: "b"}).merge({b: [1, 2]})'

After this change:

  > r.table('turtles', {readMode: 'single'}).toString()
  'r.table("turtles", {"readMode": "single"})'

  > r.expr({'-x-y-z': 2}).merge({b: [1,2]}).toString()
  'r({"-x-y-z": "b"}).merge({"b": [1, 2]})'
@deontologician deontologician self-assigned this Oct 25, 2015
@danielmewes
Copy link
Member

Thanks for the PR @mikemintz !
Have you signed http://rethinkdb.com/community/cla/ yet?

About adding a test for this: The easiest option is probably adding another mocha test to test/rql_test/connections (the folder is called connections for historic reasons, but it's really used for driver-specific tests. We'll rename it at some point). You can look at date.mocha in that folder for an example, and add a new file for toString() testing. You can run those tests through ./test-runner -i js connections in the reql_test folder.

@mglukhovsky
Copy link
Member

@mikemintz has signed the CLA (just a heads up).

@mikemintz
Copy link
Contributor Author

@danielmewes I just added tostring.mocha and confirmed that it passes only with the change made here. I'm ashamed to say I used the eval function in the test, but it seemed appropriate to avoid having to write every test query twice.

@deontologician
Copy link
Contributor

looks good to me. Any other objections to merging @danielmewes ?

@danielmewes
Copy link
Member

@deontologician Go ahead :-)

deontologician added a commit that referenced this pull request Oct 27, 2015
@deontologician
Merge pull request #4997 from mikemintz/fix-2372-javascript-tostring
99aa54a 
Properly quote JS object keys in toString()
@deontologician deontologician merged commit 99aa54a into rethinkdb:next Oct 27, 2015
@mikemintz mikemintz deleted the fix-2372-javascript-tostring branch November 9, 2015 01:08
@danielmewes danielmewes modified the milestone: 2.2 Nov 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@deontologician deontologician

Labels
None yet
Projects
None yet
Milestone
2.2
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mikemintz @danielmewes @mglukhovsky @deontologician