Permalink
Browse files

Merge pull request #21 from AdamMagaluk/master

Fix for Bug #20, uses Protobuf FieldDescriptor->name()
  • Loading branch information...
2 parents 7bf19fd + d45c173 commit 771b73d3fc98c7bea93be9b42df8e254d5927fa4 @chrisdew committed Nov 19, 2012
Showing with 2 additions and 2 deletions.
  1. +2 −2 protobuf_for_node.cc
View
@@ -143,11 +143,11 @@ namespace protobuf_for_node {
from <<
"var x = arr[" << i << "]; "
"if(x !== undefined) this['" <<
- descriptor->field(i)->camelcase_name() <<
+ descriptor->field(i)->name() <<
"'] = x; ";
if (i > 0) to << ", ";
- to << "this['" << descriptor->field(i)->camelcase_name() << "']";
+ to << "this['" << descriptor->field(i)->name() << "']";
}
from << " }})";

2 comments on commit 771b73d

Owner

chrisdew replied Nov 19, 2012

I've reversed out this merge, as npm test showed the following problem:

chris@proliant:~/github/protobuf$ npm test

> protobuf@0.8.6 test /home/chris/github/protobuf
> node test/unittest.js


assert.js:102
  throw new assert.AssertionError({
        ^
AssertionError: Number conversion
    at Object.<anonymous> (/home/chris/github/protobuf/test/unittest.js:30:8)
    at Module._compile (module.js:449:26)
    at Object.Module._extensions..js (module.js:467:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Module.runMain (module.js:492:10)
    at process.startup.processNextTick.process._tickCallback (node.js:244:9)
npm ERR! Test failed.  See above for more details.
npm ERR! not ok code 0

Contributor

seishun replied Nov 23, 2012

Yeah, because the test relies on the field names using their camel case versions. You would have to rewrite the test to use the original underscore separated names for it to pass. (please don't, this change is bad for other reasons)

Please sign in to comment.