On Sat, Dec 11, 2010 at 4:22 PM, Hartmut Goebel <h.goe...@goebel-consult.de>wrote:
> Hi, > > first of all a big thank you for applying this patch (ticket 191). Others > would not have been that brave. :-) > If we do it, we go will go faster in the future, so it's good :) > > I'm reviewed the check-in which results from it and I have some questions: > > - Why did you comment out the decorators in properties.py? > > shinken requires Python 2.4 and method decorators have been introduced in > 2.4, see > <http://docs.python.org/whatsnew/2.4.html#pep-318-decorators-for-functions-and-methods><http://docs.python.org/whatsnew/2.4.html#pep-318-decorators-for-functions-and-methods> > > I got problem to make them work (need 2 arguments, only give one) so I just comment them out and I'll try to enable them later. > > - Why are you still using these functions to_int, to_bool, etc? > > They are only adding overhead. and should be removed anyway. In case of > to_bool(), it's even worse: to_bool() only accepts '1' for a true value, > while my method allowed many other values (and would raise if unsupported > values are passed). > > Yes it add overhead, but this patch was to make dict objects, without changing the way parameters are managed. I prefer change the behaviour in another pass. But t's tru that calling another function in pythonize can be a performance problem, wo we need to "inline" them in. > > - Either 'has_default' or 'required' is redundant. Either a property > has a default value (in this case it is not required) or it is required (in > this case it doe not need a default value). > > I need one arg like it in the moment, and I didn't think about it, but yes, has_default = no required after all. > > - You introduced 'has_default' since some properties use > 'default=None'. But None is no valid default value at all. If you look at > usage of 'default=None', you'll see all (except of one) being StringProps. > And None is not a valid string, this is must not be the default for a > StringProp. > > Yes, StringProp is used every where we do not need to work on the value. I think we will need a real type for "objects" like it (do nothing, but maybe in the future we will need to work on string, so it's better to have another class). > > - Default parameters for Property.__init__() are wrong: > - managed and unmanaged are redundant (I propose removing unmanaged) > > But Unmanaged is a class isn't it? > > - > - fill_brok defaults to [] which should be None (and the list set > later) > > Why? It's easier to just "apply" the value export for all than check if it's none, then if not apply it. It make one set less. > > - no_slots=True??? > > hum... it's the default? So I'm stupid (I wanted to make the patch work on one day, I was not available this week-end) because it should be the other way. But no/no is not a good thing. We should set in_slots=True by default, and explicitly set False for some values (the one with a integer in the first character :( ). > > - > - unused is redundant. This is already stated by using UnusedProp > instance and testing isinstance(...) > > With isinstance? Yes, it's better. Let take this. > > - > - UnusedProp.managed = True?? > > To parse all objects in the managed (and not check if not unused). > > > I just uploaded a patch for fixing the issues named in the last three list > items to > <https://sourceforge.net/apps/trac/shinken/ticket/198><https://sourceforge.net/apps/trac/shinken/ticket/198> > Thanks, I'll look it :) > > As you remember I suggested rietveld for discussion patches. This is why > :-) > Emails are quite good too :) There won't be a lot of pass like it I think. If we got some quite more in the future, it will be that I was wrong, and it will be time to use it :) Thanks again for the patches and the convert tool! (same me hours of work!). The code need more clean, but the first pass was the more propblematic not we can clean all of this and then start to code new features :) Jean > -- > Schönen Gruß - Regards > Hartmut Goebel > Dipl.-Informatiker (univ.), CISSP, CSSLP > > Goebel Consult > Spezialist für IT-Sicherheit in komplexen > Umgebungenhttp://www.goebel-consult.de > > Monatliche Kolumne: http://www.cissp-gefluester.de/ > Goebel Consult mit Mitglied bei http://www.7-it.de > > > > ------------------------------------------------------------------------------ > Oracle to DB2 Conversion Guide: Learn learn about native support for > PL/SQL, > new data types, scalar functions, improved concurrency, built-in packages, > OCI, SQL*Plus, data movement tools, best practices and more. > http://p.sf.net/sfu/oracle-sfdev2dev > _______________________________________________ > Shinken-devel mailing list > Shinken-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/shinken-devel > >
------------------------------------------------------------------------------ Oracle to DB2 Conversion Guide: Learn learn about native support for PL/SQL, new data types, scalar functions, improved concurrency, built-in packages, OCI, SQL*Plus, data movement tools, best practices and more. http://p.sf.net/sfu/oracle-sfdev2dev
_______________________________________________ Shinken-devel mailing list Shinken-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/shinken-devel