Hi Petar, Thanks for the code review! Unfortunately I won't be able to touch the code for the next few weeks, so you can go ahead and apply the review comments and start using it right away if you feel like, otherwise just let me know and I'll apply the comments asap. I've just signed the electronic version of the agreement.
Thanks, Helder On Wed, Jan 21, 2009 at 9:34 AM, Petar Petrov <pe...@google.com> wrote: > On Tue, Jan 20, 2009 at 3:58 PM, Helder Suzuki <heldersuz...@gmail.com> > wrote: > > > > I sent a partial patch a while ago and I disappeared without completing > the text_format in python, sorry about that. > > So far I've only implemented the tokenizer part (w/ test cases), but > anyone is free to use it to implement the parser part (I'd be really glad), > and for some reason I couldn't set up the codereview (I didn't try hard > though). For various reasons I won't be able to touch it in the next few > weeks. > > This is such an important feature, specially if you use protobufs for > configuration files, it'd be so handy! > > Can you sign the contributor agreement: > http://code.google.com/legal/individual-cla-v1.0.html > > It's required for us to accept your patch (each contributor has to sign > it). > > I have reviewed you patch but by some reason I'm unable to send you > the comments. > > We have 2 choices: > 1. I can apply my own code review comments and submit it in the > internal repository then I will write the rest of the ParseASCII code. > 2. You can apply the code review comments below and I'll submit the > patch in external SVN. > > In any case you have to sign the agreement above. > > Here are my comments on the patch: > > google/protobuf/text_format.py: > -__all__ = [ 'MessageToString', 'PrintMessage', 'PrintField', > 'PrintFieldValue' ] > +#__all__ = ['MessageToString', 'PrintMessage', 'PrintField', > 'PrintFieldValue'] > (Draft) 2008/12/08 18:37:10 Why is this line commented? > > +class Token(object): > + """TODO(helder): Document me.""" > (Draft) 2008/12/08 18:37:50 Please add the docstrings. > > +class Tokenizer(object): > + """TODO(helder): Document me.""" > (Draft) 2008/12/08 18:39:09 1-2 sentences will be enough. > > + def ConsumeNumber(self, started_with_zero, started_with_dot): > + is_float = False > + if started_with_zero and self.TryConsumeOneOf(('x', 'X')): > + # A hex number (started with "0x"). > + self.ConsumeOneOrMore(HexDigit, '"0x" must be followed by hex digits.') > + elif started_with_zero and self.LookingAt(OctaDigit): > (Draft) 2008/12/08 19:05:42 LookingAt(OctaDigit) -> LookingAt(Digit). > It isn't a bug but a hidden feature. This will accept 09 as a decimal > 9 (will proceed with the next case - "decimal number"). I think in > this case we want to error out with a message like the one below - > "Numbers starting with leading zero must be in octal.". That's what > Kenton's implementation does. > > google/protobuf/internal/text_format_test.py: > + def testParseInteger(self): > + self.assertEqual(text_format.ParseInteger('0'), 0) > + self.assertEqual(text_format.ParseInteger('1'), 1) > + self.assertEqual(text_format.ParseInteger('012345'), 5349) # base 8 > (Draft) 2008/12/08 19:41:49 Can you also add a test which tries to > parse 09. It should result in an error. > > > > > > > > Thanks, > > Helder > > > > On Tue, Dec 30, 2008 at 12:19 AM, Piotr Findeisen < > piotr.findei...@gmail.com> wrote: > >> > >> > >> > >> On Dec 8, 7:27 pm, Petar Petrov <pe...@google.com> wrote: > >> > On Mon, Dec 8, 2008 at 10:21 AM, Kenton Varda <ken...@google.com> > wrote: > >> > > Hey Petar, isn't there a patch someone was trying to submit that > implements > >> > > text format parsing? (For real, not by wrapping protoc.) What's > the status > >> > > of that? > >> > > >> > I'll review it today. > >> > Hopefully the author hasn't forgotten about it. > >> > >> Hey! > >> This gonna be a feature I miss really much! > >> Is there happening anything about this? > >> > >> regards! > >> Piotr > >> > >> > >> > > > --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Protocol Buffers" group. To post to this group, send email to email@example.com To unsubscribe from this group, send email to protobuf+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/protobuf?hl=en -~----------~----~----~----~------~----~------~--~---