FastBinaryEncoding icon indicating copy to clipboard operation
FastBinaryEncoding copied to clipboard

python uuid default value small bug

Open PengyiPan opened this issue 2 years ago • 3 comments

In latest version 1.12.0.0, when generating python files

example .fbe file

message Generated
{
     string val1 = "42";
     uuid [id] = uuid1;
}

will create something like

...
class Generated(object):
     ...
     def __init__(self, val1=None, id=uuid.uuid1()):
          ...

which actually fixes the id to be the same when file is first evaluated, so the following code will print the same value

g1 = Generated()
g2 = Generated()
print(g1.id)
print(g2.id)

and I think this behaviour is inconsistent with its C++ counterpart.

Proposed fix:

...
class Generated(object):
     ...
     def __init__(self, val1=None, id=None):
          ...
          if id is None:
              id = uuid.uuid1()

This error may also appear for other type of default value, for example '[]'(empty list), etc. I have to say this python feature is quite counter-intuitive.

see also: https://stackoverflow.com/questions/1132941/least-astonishment-and-the-mutable-default-argument

PengyiPan avatar Sep 28 '22 12:09 PengyiPan

Thanks for reporting! Fixed in 1.13.0.0

chronoxor avatar Sep 28 '22 15:09 chronoxor

Thanks for reporting! Fixed in 1.13.0.0 #

Thanks for the fast reply! @chronoxor It's the fastest I've ever seen in any open-source project.

However, after trying the 1.13, I think you missed one small thing:

...
class Generated(object):
     ...
     def __init__(self, val1=None, id=None):
          ...
          if id is None:
              uuid.uuid1()   # here!  should be id = uuid.uuid1()?

not sure about whether this fix will cause side effects: in generator_python.cpp, line 3316

// Before
 WriteLineIndent(ConvertConstant(*field->type, *field->value, field->optional));

// After
 WriteLineIndent(*field->name + " = " + ConvertConstant(*field->type, *field->value, field->optional));

PengyiPan avatar Sep 29 '22 03:09 PengyiPan

Fixed as well

chronoxor avatar Sep 29 '22 12:09 chronoxor