Thanks for your comments ced, no doubt you know the best about trytond internals.
although i am perfectly fine to accept that not a piece of my work will make it to upstream, please let me On Tuesday, September 2, 2014 11:07:44 PM UTC+2, Cédric Krier wrote: > > On 02 Sep 09:14, simon....@gmail.com <javascript:> wrote: > > hi there, > > > > first thing, im sorry for doing this in reverse order: i have already > > opened a codereview at http://codereview.tryton.org/10481002/ and am > now > > writing in here to state my motivation. > > > > > > the reason i even took a deeper look at modules/__init__.py was that i > > wanted to run tryton in a more project/framework oriented way: having a > > project folder with tryton modules and my own modules, similar to how a > > common django-project would look like. This would ease the process of > > developing (getting started using sqlite straight away) and creating > > docker-instances a lot for me. > > This is a wrong goal and will not land in Tryton because: > > - it does enforce uniqueness of module name > Really, did you ever try to load more than one module with non-unique names? I doubt you would ever succeed on this for quite some reasons: [including links :) ] - http://hg.tryton.org/trytond/file/f0e247a7b960/trytond/modules/__init__.py#l76 - http://hg.tryton.org/trytond/file/f0e247a7b960/trytond/modules/__init__.py#l139 (here you would get the same config file on all those) - http://hg.tryton.org/trytond/file/f0e247a7b960/trytond/modules/__init__.py#l296 maybe there is even more, i think that is enough stones for a developer who tries to use two modules with the same name (anyhow i see no reason a sane person would do that) with the patch it would instead be possible to replace the internal modules (ir, webdav,..) if a developer really wants to - it is useless because modules can be registered via pkg_resources. > > True. but then i still have to add a setup.py and install using pip, which turns out to be not soo simple to automate with docker.. with the patch it is just creating a new directory with a tryton.cfg to trigger a restart of the server... also it is much faster to bootstrap a new project with sqlite. > > > So i started to look into the module-loading and thought it should not > be > > too hard to add more module-directories to it. But reality turned out to > be > > the opposite, because the logic was really hard to understand and very > > weird at certain points, some examples: > > > > - 226-231: ! > > Simple it sets the right state. > indeed it sets the right state, but it looks like a perfect antipattern: if a or b: if not b: making 2 same-level ifs requires less mind-twisting... > > - 332-333: its really hard to find out what 'childs' are because its > strange how they are set/found. second is that once i found out that its > actually modules which have the current one as a dependency it seemed > really strange that the logic did not apply on extras_depends? > > Don't see the lines. (Refering to line numbers is useless if we don't > have the version, so use a link) > sorry i mixed the numbers (using a link avoids that, thanks for the hint!) http://hg.tryton.org/trytond/file/f0e247a7b960/trytond/modules/__init__.py#l233 > It is a oriented graph so yes the childs are named childs. > > > - the searching of module-paths happens a few times spread all over > trytond (modules/__init__, tools/misc,..) > > Please show examples. > > http://hg.tryton.org/trytond/file/f0e247a7b960/trytond/modules/__init__.py#l295 http://hg.tryton.org/trytond/file/f0e247a7b960/trytond/modules/__init__.py#l334 -356 http://hg.tryton.org/trytond/file/f0e247a7b960/trytond/monitor.py#l63 (its not known, thats why many so many directories are searched - maybe not the best example) http://hg.tryton.org/trytond/file/f0e247a7b960/trytond/tools/misc.py#l83 > - the only reason to introduce a graph and (singleton-) nodes is to > have pretty-print? > > No, look they are methods. > > what is the method? > > However, i ended up reverse-engineering > > reverse-engineering with the source code :-) > your point :) http://bit.ly/1qcmAUi with the source you can look from above... > > and rewriting the whole thing and > > The best way to not have your contribution accepted. > Why will we accept to maintain your new code instead of existing one? > it is still more of a proposal to make the affected part a little more straightforward. If you prefer to maintain the current state, please, your choice! What are the proofs that your code is better? What does it fix? it does not fix an existing issue it contains less magic and more docstrings, which i personally do prefer. The intention was to share my thoughts on how a clean re-structurisation could! look like, not to offend you with my hipster-dipster refractoring. > By reading your comments, I see more that your code introduced bugs than > fixing. > > true that it did introduce "bugs" when i was playing around using a recursive approach, i marked it to do so and changed it back, i uploaded too early, sorry for that! > > the result is in the stated codereview... > > > > the concept is to have a module-index which crawls all places containing > > modules on request and can create lists of modules sorted by > dependencies. > > this index holds all the modules and therefore things like file_open > became > > obsolete because the module-path is known all the time. > > file_open is any way needed by other functionalities. > > $ grep -r "file_open" . $ grep -r "file_open" . -A 10 | grep subdir which "functionalities" do you mean? > the first approach to create a sorted list was recursive which should > have > > still been valid, but the order was different which broke tests from > > stock_lot (databases arent dropped and thereby sale was installed > > already..), thats why in the meantime it is iterative again > > So your tree is not the same as the current one so yours is not > compatible. > it "used to be" > > > together with http://codereview.tryton.org/10511002/ even more > > simplification could apply to it, but excuse me, if i might have been > too > > aggressive resulting from the frustration of being rejected so harsh on > > such a simple thing... (i still fail to understand the difference > between > > builtin-modules and 'modules') > > This is not simple thing and you can not explain what you are fixing. > > doing absolute imports even inside a package is just as simple as doing absolute imports. - it would allow further simplification at a later point, - it comes with no cost (except for spamming the commit tree) - it is recommended by pep8 - it would clarify the source-reader that (ir, res, webdav, tests) is like any other module, just internal > please dont get me wrong, i dont want to reinvent the wheel, but i wonder > > how this is maintainable if its hardly understandable (it seems like it > > roots in openerp era) > > For me, the code is understandable but of course there are room for > improvement but not as a big re-write because it is very delicate > subject. > So if you want to improve and contribute, please do it small step by > small step instead of telling us: I don't understand the code so I > rewrote it. > I'm really sorry i did, but i did not say "hey! i want this upstream next week" All i did was to claim that in its current state it really does lack in structure and is overengineered for what it really does. I understand that this is a core part and i appreciate you dont just take what an untrusted person writes, but i think you have to admit that the current code is far away from keeping things simple and it really needs some structure to be predictable and therefore maintainable. best regards! simon
