fix proposal for issue 1575

[ced, 2010-05-28 15:58:40]

http://codereview.appspot.com/1387041/diff/1/2
File trytond/protocols/datatype.py (right):

http://codereview.appspot.com/1387041/diff/1/2#newcode4
trytond/protocols/datatype.py:4: class Float(float):
I think we should inherit from Decimal to be considered also as a Decimal

http://codereview.appspot.com/1387041/diff/1/2#newcode21
trytond/protocols/datatype.py:21: if isinstance(other, Decimal) and
hasattr(self, 'decimal'):
It will always have decimal attribute

http://codereview.appspot.com/1387041/diff/1/2#newcode22
trytond/protocols/datatype.py:22: res = Decimal(self.decimal).__mul__(other)
Missing import Decimal

[ced, 2010-05-29 06:20:58]

You should also write unittest

[ced, 2010-05-31 19:05:06]

still missing unittest

http://codereview.appspot.com/1387041/diff/6001/7001
File trytond/protocols/datatype.py (right):

http://codereview.appspot.com/1387041/diff/6001/7001#newcode5
trytond/protocols/datatype.py:5: class Float(Decimal):
It should be:

Float(Decimal, float)

[ced, 2010-05-31 22:17:27]

Also don't forget to pass "--send_mail" to upload.py otherwise reviewers are not
notified of new version.

[ced, 2010-06-01 09:23:28]

You must also add the new test case in test_tryton.py

http://codereview.appspot.com/1387041/diff/10001/11001
File trytond/protocols/datatype.py (right):

http://codereview.appspot.com/1387041/diff/10001/11001#newcode5
trytond/protocols/datatype.py:5: class Float(Decimal):
Still missing float inherit

http://codereview.appspot.com/1387041/diff/10001/11002
File trytond/tests/test_datatype.py (right):

http://codereview.appspot.com/1387041/diff/10001/11002#newcode16
trytond/tests/test_datatype.py:16: class FloatTestCase(unittest.TestCase):
Better to named it ProtocolsDatatypeFloatTestCase

http://codereview.appspot.com/1387041/diff/10001/11002#newcode25
trytond/tests/test_datatype.py:25:
self.assert_(Float('1.0').__mul__(Decimal('1.0')) == Decimal('1.0'))
Better to test the operator instead of calling the function

[gadaga, 2010-06-01 10:08:19]

http://codereview.appspot.com/1387041/diff/10001/11001
File trytond/protocols/datatype.py (right):

http://codereview.appspot.com/1387041/diff/10001/11001#newcode5
trytond/protocols/datatype.py:5: class Float(Decimal):
On 2010/06/01 09:23:29, ced wrote:
> Still missing float inherit

if we inherit Decimal and float, we've got an error:
    class Float(Decimal, float):
TypeError: Error when calling the metaclass bases
    multiple bases have instance lay-out conflict

[ced, 2010-06-01 10:26:07]

On 2010/06/01 10:08:19, gadaga wrote:
> http://codereview.appspot.com/1387041/diff/10001/11001
> File trytond/protocols/datatype.py (right):
> 
> http://codereview.appspot.com/1387041/diff/10001/11001#newcode5
> trytond/protocols/datatype.py:5: class Float(Decimal):
> On 2010/06/01 09:23:29, ced wrote:
> > Still missing float inherit
> 
> if we inherit Decimal and float, we've got an error:
>     class Float(Decimal, float):
> TypeError: Error when calling the metaclass bases
>     multiple bases have instance lay-out conflict

I think you must overide __new__

[ced, 2010-06-01 10:39:18]

http://codereview.appspot.com/1387041/diff/18001/19001
File trytond/protocols/datatype.py (right):

http://codereview.appspot.com/1387041/diff/18001/19001#newcode5
trytond/protocols/datatype.py:5: class Float(Decimal):
It misses:

__pow__
__rpow__
__long__
__complex__
__int__
__float__
__floordiv__
__mod__
__rmod__
__divmod__
__rdivmod__
__abs__
__pos__
__neg__
__hash__
__ge__
__gt__
__le__
__lt__
__ne__
__eq__
__nonzero__

[B.V., 2010-06-01 11:37:10]

On 2010/06/01 10:26:07, ced wrote:
> On 2010/06/01 10:08:19, gadaga wrote:
> > http://codereview.appspot.com/1387041/diff/10001/11001
> > File trytond/protocols/datatype.py (right):
> > 
> > http://codereview.appspot.com/1387041/diff/10001/11001#newcode5
> > trytond/protocols/datatype.py:5: class Float(Decimal):
> > On 2010/06/01 09:23:29, ced wrote:
> > > Still missing float inherit
> > 
> > if we inherit Decimal and float, we've got an error:
> >     class Float(Decimal, float):
> > TypeError: Error when calling the metaclass bases
> >     multiple bases have instance lay-out conflict
> 
> I think you must overide __new__

It won't be enough because type.__new__(type, "Alien", (Decimal, float), {})
raises a TypeError.
I think playing with __metaclass__ won't tackle the problem either.

[ced, 2010-06-01 11:53:30]

On 2010/06/01 11:37:10, B.V. wrote:
> On 2010/06/01 10:26:07, ced wrote:
> > On 2010/06/01 10:08:19, gadaga wrote:
> > > http://codereview.appspot.com/1387041/diff/10001/11001
> > > File trytond/protocols/datatype.py (right):
> > > 
> > > http://codereview.appspot.com/1387041/diff/10001/11001#newcode5
> > > trytond/protocols/datatype.py:5: class Float(Decimal):
> > > On 2010/06/01 09:23:29, ced wrote:
> > > > Still missing float inherit
> > > 
> > > if we inherit Decimal and float, we've got an error:
> > >     class Float(Decimal, float):
> > > TypeError: Error when calling the metaclass bases
> > >     multiple bases have instance lay-out conflict
> > 
> > I think you must overide __new__
> 
> It won't be enough because type.__new__(type, "Alien", (Decimal, float), {})
> raises a TypeError.
> I think playing with __metaclass__ won't tackle the problem either.

But we need to have:

isinstance(Float('1'), float) == True
isinstance(Float('1'), Decimal) == True

[B.V., 2010-06-01 12:15:37]

On 2010/06/01 11:53:30, ced wrote:
> On 2010/06/01 11:37:10, B.V. wrote:
> > On 2010/06/01 10:26:07, ced wrote:
> > > On 2010/06/01 10:08:19, gadaga wrote:
> > > > http://codereview.appspot.com/1387041/diff/10001/11001
> > > > File trytond/protocols/datatype.py (right):
> > > > 
> > > > http://codereview.appspot.com/1387041/diff/10001/11001#newcode5
> > > > trytond/protocols/datatype.py:5: class Float(Decimal):
> > > > On 2010/06/01 09:23:29, ced wrote:
> > > > > Still missing float inherit
> > > > 
> > > > if we inherit Decimal and float, we've got an error:
> > > >     class Float(Decimal, float):
> > > > TypeError: Error when calling the metaclass bases
> > > >     multiple bases have instance lay-out conflict
> > > 
> > > I think you must overide __new__
> > 
> > It won't be enough because type.__new__(type, "Alien", (Decimal, float), {})
> > raises a TypeError.
> > I think playing with __metaclass__ won't tackle the problem either.
> 
> But we need to have:
> 
> isinstance(Float('1'), float) == True
> isinstance(Float('1'), Decimal) == True

Yes. And I need 10,000,000.00 EUR on my bank account.

More seriously, we may give a try on python forums or mailing lists to be sure
what you ask is possible or not.

And if there's no solution, I think we may have to change something else. Maybe
only handle Decimal in trytond and get rid of float.

[B.V., 2010-06-02 12:32:04]

On 2010/06/01 12:15:37, B.V. wrote:
> On 2010/06/01 11:53:30, ced wrote:
> > On 2010/06/01 11:37:10, B.V. wrote:
> > > On 2010/06/01 10:26:07, ced wrote:
> > > > On 2010/06/01 10:08:19, gadaga wrote:
> > > > > http://codereview.appspot.com/1387041/diff/10001/11001
> > > > > File trytond/protocols/datatype.py (right):
> > > > > 
> > > > > http://codereview.appspot.com/1387041/diff/10001/11001#newcode5
> > > > > trytond/protocols/datatype.py:5: class Float(Decimal):
> > > > > On 2010/06/01 09:23:29, ced wrote:
> > > > > > Still missing float inherit
> > > > > 
> > > > > if we inherit Decimal and float, we've got an error:
> > > > >     class Float(Decimal, float):
> > > > > TypeError: Error when calling the metaclass bases
> > > > >     multiple bases have instance lay-out conflict
> > > > 
> > > > I think you must overide __new__
> > > 
> > > It won't be enough because type.__new__(type, "Alien", (Decimal, float),
{})
> > > raises a TypeError.
> > > I think playing with __metaclass__ won't tackle the problem either.
> > 
> > But we need to have:
> > 
> > isinstance(Float('1'), float) == True
> > isinstance(Float('1'), Decimal) == True
> 
> Yes. And I need 10,000,000.00 EUR on my bank account.
> 
> More seriously, we may give a try on python forums or mailing lists to be sure
> what you ask is possible or not.
> 
> And if there's no solution, I think we may have to change something else.
Maybe
> only handle Decimal in trytond and get rid of float.

I have few feedbacks for the messages I post (0 on afpy forum [1] and 1 on
comp.lang.python [2]).

On c.l.p., Mark Dickinson wrote: """I don't think your approach can succeed; 
I'd suggest just subclassing 
'object' and abandoning the 'isinstance' requirements.  Or perhaps 
creating a subclass of Decimal that interacts nicely with floats.""".

[1]: http://www.afpy.org/python/forum_python/forum_general/0454358559868
[2]:
http://groups.google.com/group/comp.lang.python/browse_thread/thread/65f5a800...

[ced, 2010-06-03 20:11:27]

On 2010/06/02 12:32:04, B.V. wrote:
> On 2010/06/01 12:15:37, B.V. wrote:
> > On 2010/06/01 11:53:30, ced wrote:
> > > On 2010/06/01 11:37:10, B.V. wrote:
> > > > On 2010/06/01 10:26:07, ced wrote:
> > > > > On 2010/06/01 10:08:19, gadaga wrote:
> > > > > > http://codereview.appspot.com/1387041/diff/10001/11001
> > > > > > File trytond/protocols/datatype.py (right):
> > > > > > 
> > > > > > http://codereview.appspot.com/1387041/diff/10001/11001#newcode5
> > > > > > trytond/protocols/datatype.py:5: class Float(Decimal):
> > > > > > On 2010/06/01 09:23:29, ced wrote:
> > > > > > > Still missing float inherit
> > > > > > 
> > > > > > if we inherit Decimal and float, we've got an error:
> > > > > >     class Float(Decimal, float):
> > > > > > TypeError: Error when calling the metaclass bases
> > > > > >     multiple bases have instance lay-out conflict
> > > > > 
> > > > > I think you must overide __new__
> > > > 
> > > > It won't be enough because type.__new__(type, "Alien", (Decimal, float),
> {})
> > > > raises a TypeError.
> > > > I think playing with __metaclass__ won't tackle the problem either.
> > > 
> > > But we need to have:
> > > 
> > > isinstance(Float('1'), float) == True
> > > isinstance(Float('1'), Decimal) == True
> > 
> > Yes. And I need 10,000,000.00 EUR on my bank account.
> > 
> > More seriously, we may give a try on python forums or mailing lists to be
sure
> > what you ask is possible or not.
> > 
> > And if there's no solution, I think we may have to change something else.
> Maybe
> > only handle Decimal in trytond and get rid of float.
> 
> I have few feedbacks for the messages I post (0 on afpy forum [1] and 1 on
> comp.lang.python [2]).
> 
> On c.l.p., Mark Dickinson wrote: """I don't think your approach can succeed; 
> I'd suggest just subclassing 
> 'object' and abandoning the 'isinstance' requirements.  Or perhaps 
> creating a subclass of Decimal that interacts nicely with floats.""".
> 
> [1]: http://www.afpy.org/python/forum_python/forum_general/0454358559868
> [2]:
>
http://groups.google.com/group/comp.lang.python/browse_thread/thread/65f5a800...

So let's go for float instance

[ced, 2010-06-04 08:45:44]

http://codereview.appspot.com/1387041/diff/28001/29001
File trytond/protocols/datatype.py (right):

http://codereview.appspot.com/1387041/diff/28001/29001#newcode7
trytond/protocols/datatype.py:7: super(Float, self).__init__()
Missing value to __init__

http://codereview.appspot.com/1387041/diff/28001/29001#newcode8
trytond/protocols/datatype.py:8: self.__decimal = value
Would be better to store Decimal value

http://codereview.appspot.com/1387041/diff/28001/29001#newcode20
trytond/protocols/datatype.py:20: def __mul__(self, other):
Missing context

http://codereview.appspot.com/1387041/diff/28001/29001#newcode21
trytond/protocols/datatype.py:21: if isinstance(other, Decimal):
Code can be shorter:

if isinstance(other, Decima):
   return self.decimal.__mul__(other, context=context)
return float.__mul__(self, other)

http://codereview.appspot.com/1387041/diff/28001/29002
File trytond/tests/test_datatype.py (right):

http://codereview.appspot.com/1387041/diff/28001/29002#newcode103
trytond/tests/test_datatype.py:103: self.assert_(Float('2.0') - float('1.0') ==
float('1.0'))
float('1.0') can be written as 1.0

[gadaga, 2010-06-04 09:32:48]

http://codereview.appspot.com/1387041/diff/28001/29001
File trytond/protocols/datatype.py (right):

http://codereview.appspot.com/1387041/diff/28001/29001#newcode7
trytond/protocols/datatype.py:7: super(Float, self).__init__()
On 2010/06/04 08:45:44, ced wrote:
> Missing value to __init__
__init__ on float class don't take parameters, if we passe it a parameter we
have a deprecation warning

[ced, 2010-06-04 10:18:02]

I also think it will be more readable to define arithmetic functions in
alphabetic order.

http://codereview.appspot.com/1387041/diff/34001/35002
File trytond/protocols/datatype.py (right):

http://codereview.appspot.com/1387041/diff/34001/35002#newcode22
trytond/protocols/datatype.py:22: return self.decimal.__mul__(other, context)
Pass keyword arguments it is more robust to change

[ced, 2010-06-04 12:23:33]

On 2010/06/04 10:29:54, gadaga wrote:
> 

I will put roperator just under operator
And also set the same order for tests

[ced, 2010-06-04 12:25:44]

Still missing all the other functions

[ced, 2010-06-04 13:59:40]

Do you plan to add missing methods?
If not tell me, I will do it.

[gadaga, 2010-06-04 14:16:45]

On 2010/06/04 13:59:40, ced wrote:
> Do you plan to add missing methods?
> If not tell me, I will do it.
Yes, i'll do it, but i've got a lot of work today...

[ced, 2010-06-04 18:08:31]

On 2010/06/04 14:16:45, gadaga wrote:
> On 2010/06/04 13:59:40, ced wrote:
> > Do you plan to add missing methods?
> > If not tell me, I will do it.
> Yes, i'll do it, but i've got a lot of work today...

Ok, I will continue the patch this WE

[ced, 2010-08-22 17:49:35]

Can be closed.

[ced, 2010-12-04 16:22:25]

Please close it as it stays in the queue of patch to review and this is
annoying.
Reviewers can not edit the issue.