Re: [tryton-dev] Active Record the next step
I tried to have a look at everything, though as a tryton newcommer
I admit that I could not understand perfectly.
Maybe you should upgrade a simple (or dummy) module and make
a before-after comparison ? I saw bits of it in a few (mostly) test
classes, but it often is renaming _name, removing _description
and adding @staticmethod / @classmethod everywhere.
No need yet for a review. It is more about the design.
I like this design. It looks and feels cleaner and easier to use.
One little question anyway :
* Return objects instead of id's in create, search, etc.
in trytond/tests/tests_wizard.py,
+ group_a_id = self.group.create({ + 'name': 'Group A', + })Which one is
right ?
Jean CAVALLO
--
tryton-dev@googlegroups.com mailing list
Re: [tryton-dev] Active Record the next step
On 30/04/12 11:08 +0200, Jean Cavallo wrote:
I tried to have a look at everything, though as a tryton newcommer
I admit that I could not understand perfectly.
Maybe you should upgrade a simple (or dummy) module and make
a before-after comparison ? I saw bits of it in a few (mostly) test
classes, but it often is renaming _name, removing _description
and adding @staticmethod / @classmethod everywhere.
There are modules: ir, res and webdav.
No need yet for a review. It is more about the design.
I like this design. It looks and feels cleaner and easier to use.
One little question anyway :
* Return objects instead of id's in create, search, etc.
in trytond/tests/tests_wizard.py,
+ group_a_id = self.group.create({ + 'name': 'Group A', + })Which one is
right ?
The changelog.
PS: Be careful, gmail has a poor html2text converter so prefer to post
in plain text on mailing list (see how it looks weird).
--
Cédric Krier
B2CK SPRL
Rue de Rotterdam, 4
4000 Liège
Belgium
Tel: +32 472 54 46 59
Email/Jabber: cedric.kr...@b2ck.com
Website: http://www.b2ck.com/
pgpRbg8G9xASR.pgp
Description: PGP signature
Re: [tryton-dev] Active Record the next step
Maybe you should upgrade a simple (or dummy) module and make a before-after comparison ? I saw bits of it in a few (mostly) test classes, but it often is renaming _name, removing _description and adding @staticmethod / @classmethod everywhere. There are modules: ir, res and webdav. I meant something simpler, those are rather core modules. For instance the country or currency module. PS: Be careful, gmail has a poor html2text converter so prefer to post in plain text on mailing list (see how it looks weird). My mistake, I won't do it again. -- tryton-dev@googlegroups.com mailing list
Re: [tryton-dev] Active Record the next step
A Dimecres, 25 d'abril de 2012 13:46:18, Cédric Krier va escriure: On 25/04/12 11:37 +, cedric.kr...@b2ck.com wrote: Please review this at http://codereview.tryton.org/346003/ This is a first draft for our next step to the Active Record pattern. Let's talk here about the design. Here are the main changes: * Add RPC * Repace BrowseRecord by Model instance * Replace Cache decorator by a simple LRU Cache * Remove Cacheable * Remove _description * Rename _name by __name__ * Use class in Pool I like the new design: Removing _description for __doc__ looks nice. The same for using the class in Pool or replacing _name by __name__. Just several comments: It has its technical reasons, but it is a bit complicated to figure out how each kind of function should be defined. For example, default_* use @staticmethod, get/set and on_change use standard functions (but may use @classmethod), while check_*, read, write, etc must use @classmethod. When you think of it, you can find out why each one uses each kind of definition, but It makes coding a little bit harder, specially for newcommers... Not that I have a better solution, but wanted to point this out. Also the fact that some get functions of fields.Function use @classmethod and others use standard functions makes it a bit more complex. Could we standarize and always use the same mechanism? Is there a change to keep the Cache decorator? I think it makes code cleaner. I know we prefer explicit vs implicit but it be nice if we didn't have to register all classes :) -- Albert Cervera i Areny http://www.NaN-tic.com Tel: +34 93 553 18 03 http://twitter.com/albertnan http://www.nan-tic.com/blog -- tryton-dev@googlegroups.com mailing list
Re: [tryton-dev] Active Record the next step
On 29/04/12 19:16 +0200, Albert Cervera i Areny wrote: A Dimecres, 25 d'abril de 2012 13:46:18, Cédric Krier va escriure: On 25/04/12 11:37 +, cedric.kr...@b2ck.com wrote: Please review this at http://codereview.tryton.org/346003/ This is a first draft for our next step to the Active Record pattern. Let's talk here about the design. Here are the main changes: * Add RPC * Repace BrowseRecord by Model instance * Replace Cache decorator by a simple LRU Cache * Remove Cacheable * Remove _description * Rename _name by __name__ * Use class in Pool I like the new design: Removing _description for __doc__ looks nice. The same for using the class in Pool or replacing _name by __name__. Just several comments: It has its technical reasons, but it is a bit complicated to figure out how each kind of function should be defined. For example, default_* use @staticmethod, get/setand on_change use standard functions (but may use @classmethod), while check_*, read, write, etc must use @classmethod. When you think of it, you can find out why each one uses each kind of definition, but It makes coding a little bit harder, specially for newcommers... Not that I have a better solution, but wanted to point this out. We will update the documentation to clarify each cases. (check_* should also work like getter of fields.Function) Also the fact that some get functions of fields.Function use @classmethod and others use standard functions makes it a bit more complex. Could we standarize and always use the same mechanism? Indeed the simplier is the instance method but that is not always the best because it could be better to work on list like when you can do a SQL query to compute all at once. Is there a change to keep the Cache decorator? I think it makes code cleaner. I don't think. There was an issue with the decorator see issue1825 [1]. Also the code of the decorator was a little bit magic. And I'm pretty sure we will find new way to use this implementation (already it merge the decorator with the Mixin). I know we prefer explicit vs implicit but it be nice if we didn't have to register all classes :) But it is more powerful because if we want to follow the module organisation (extension in file named like module), we must be able to give a register order different than the importation one. Also it will be possible to register generated classes or based on condition. Other minor advantage, it remove pyflakes warnings :-) [1] https://bugs.tryton.org/issue1825 -- Cédric Krier B2CK SPRL Rue de Rotterdam, 4 4000 Liège Belgium Tel: +32 472 54 46 59 Email/Jabber: cedric.kr...@b2ck.com Website: http://www.b2ck.com/ pgp5D7SnqDvZc.pgp Description: PGP signature
Re: [tryton-dev] Active Record the next step
A Diumenge, 29 d'abril de 2012 20:17:25, Cédric Krier va escriure: Also the fact that some get functions of fields.Function use @classmethod and others use standard functions makes it a bit more complex. Could we standarize and always use the same mechanism? Indeed the simplier is the instance method but that is not always the best because it could be better to work on list like when you can do a SQL query to compute all at once. Yes, I saw that, but maybe it is simpler to make all functions behave the same way (that is the version that can be optimized). Maybe I need to get used to it, but it seems that it's harder to read the code if each function follows a different scheme. Maybe it's better to always consider you're getting the list of models and that's it. -- Albert Cervera i Areny http://www.NaN-tic.com Tel: +34 93 553 18 03 http://twitter.com/albertnan http://www.nan-tic.com/blog -- tryton-dev@googlegroups.com mailing list
Re: [tryton-dev] Active Record the next step
On 29/04/12 20:45 +0200, Albert Cervera i Areny wrote: A Diumenge, 29 d'abril de 2012 20:17:25, Cédric Krier va escriure: Also the fact that some get functions of fields.Function use @classmethod and others use standard functions makes it a bit more complex. Could we standarize and always use the same mechanism? Indeed the simplier is the instance method but that is not always the best because it could be better to work on list like when you can do a SQL query to compute all at once. Yes, I saw that, but maybe it is simpler to make all functions behave the same way (that is the version that can be optimized). Maybe I need to get used to it, but it seems that it's harder to read the code if each function follows a different scheme. Maybe it's better to always consider you're getting the list of models and that's it. I don't agree. It is because you can use the right kind of method that makes the code easier to read. -- Cédric Krier B2CK SPRL Rue de Rotterdam, 4 4000 Liège Belgium Tel: +32 472 54 46 59 Email/Jabber: cedric.kr...@b2ck.com Website: http://www.b2ck.com/ pgphBIvcSUQk7.pgp Description: PGP signature
Re: [tryton-dev] Active Record the next step
On 28/04/12 17:10 +0200, Albert Cervera i Areny wrote: A Dissabte, 28 d'abril de 2012 16:41:35, Cédric Krier va escriure: On 25/04/12 13:46 +0200, Cédric Krier wrote: Really ! Still no comments? Guys, I need to know your thoughts and ideas to improve it before starting patches for modules. Well, the patch is huuuge :) Just started to review it. Hope to be able to give feedback tomorrow. No need yet for a review. It is more about the design. BTW, did you make any performance comparison? For example, to see if returning objects in search() or create() affect performance noticeably. Not really, but I did not see any slow on my host (which is very cheap and so most of the time, I see directly low performance). -- Cédric Krier B2CK SPRL Rue de Rotterdam, 4 4000 Liège Belgium Tel: +32 472 54 46 59 Email/Jabber: cedric.kr...@b2ck.com Website: http://www.b2ck.com/ pgpeCa7fwYFoN.pgp Description: PGP signature
Re: [tryton-dev] Active Record the next step
Well, the patch is huuuge :) +1, I'm on it but I will need a few days to go through everything. Jean -- tryton-dev@googlegroups.com mailing list
