Dear Gustavo, As I said in IRC yesterday: my intention is *not* to insult you or your code in any way. Whatever I said here is general and could have been applied to anyone's code. If I insulted you, I'm sorry.
Now for my comments. On 03/04/12 17:58, Gustavo Sverzut Barbieri wrote: > Discomfitor played with it, as far as I remember he liked its usage in esql. > > The code is stupidly simple and my major concern was with memory usage > and speed. Both were addressed nicely, we can handle structures and > arrays in a fast and space efficient way. > > Its purpose is clear: hold a generic typed value. Then it keeps the > value memory and the pointer to its operations. The operations were > added based on other languages and toolkits. They cover copy, compare > and convert, aside from basic set and get. > As I said, I'm against untested/reviewed/played with API. If this code was indeed tested, played with and "approved" by discomiftor, it's fine. Lets focus our discussion on eina_model then. > What API? It's a model. Composed of named (string) properties with > generic values (strong typed with Eina_Value). Children is a sequence > of other Eina_Model. > > The API clearly reflects that. They are clearly defined at: > > > http://docs.enlightenment.org/auto/eina/group__Eina__Model__Properties__Group.html > > http://docs.enlightenment.org/auto/eina/group__Eina__Model__Children__Group.html > > Other than that, it comes with common use cases in an easy to use way, > like filtering items, searching, sorting and iterating. All of them > based on real world use cases. > > There are default implementations for these using super-generic array > + hash, and efficient array + struct. One is able to override the > operations in order to optimize use-cases, such as talking directly > with DB, or abstract an already existing in-memory data structure. > > The focus is on how to use the common model access. The properties and > children listed above. Do you see anything wrong with them? > > The inheritance and all is a nice thing that came and I exposed them, > but they should not be the focus. They look good in my point of view, > nobody could spot a problem with them, but the inheritance is > secondary to the model usage as a consumer. I haven't played with properties or children enough to comment on them, and that's the problem. :) Also, if the inheritance stuff are not the important part, why are they so heavily exposed and make a big part of the API? > > > Indeed we did had this model thing for a while (since 2008), but the > eina version was written based on real world requirements from > scratch. Then we're not just releasing our code, we did plan it to > benefit everybody. > > I can't get your points of "What's the rush?" I'm not rushing it, it's > there in SVN for a while (+2 months) and I'm just following the EFL > standards of having things on trunk for testing. Raster himself > believes this is the only way to get things tested, so I did it. > Should it go in a branch? There are not branches for other work AFAIK. Yeah, that's what raster thinks is right (me as well), but it depends on the code actually being reviewed and not just sitting there without any attention. I'm not saying it should go in a branch, just "disabled" for the *current* efl release and re-activated and discussed right afterwards. Rush: as I said, I don't think there's any urgent need of that, so getting that in although it was not tested (API) enough is rushing. > > > Could you list the real world example? The eina_model_04 is not a real > world example of DATA model as far as I understand it. Could you > present us with such example? > > Also, the "forced unneeded dep" will still work in future. It will not > be required anymore, but it will still work if that's what concerns > you. > > I fail to see how it breaks API or ABI. The same code will work. Let's > say now there are undefined behaviors, that can be defined later. > POSIX libc is full of such. I take your point there, it doesn't break API, it just encourages abusing the API. Real world example: you want to override the child_set function of the Children interface to filter forbidden children, for example... It's just function overriding for interfaces, it's common. > > > Uh? You gave python as an example of a project that got stuck with API they didn't like for ages and that they changed it after a long while. I said that we wouldn't want to repeat their errors. > Yes, but the existing code still works :-) You were relying on an > undefined behavior by current means. It may be defined later IMO. > > All in all the code is simple to write and can be done. But should we? Yes, but this specific piece of code (without that unneeded dep) will change behaviour and not work after you fix it. You can argue that whoever wrote that code was relying on undefined behaviour, and I'll claim accordingly that we haven't "explored" eina_model enough to know what's defined/expected and what's not. Also, we can't yet foresee if this API will suite our near future needs. > > Which example? Your example tried to use model as general object > system to define hierarchy. It does not map any data into anything. > Again: model is about data. I gave an example above. If model is about data, ditch the inheritance, if you want to support inheritance, do it right. You can't claim this is a stupid feature that should not be really used... >> Because it was not tested enough. But as I said, there are some other things >> that are wrong in the API. I'm 100% sure I've told you about them in IRC, >> but maybe also in mail. It's still rough. > > Are you serious? Really? Please review the last changes to all EFL. > From Eina to Elementary. If I would act like this with you, lots of > code would not get in past releases as many things were added in a > similar way. Do you remember binbuf? How about binshare? Many changes > to Evas text rendering. They got in, to get tested, and now they rock > and are very usable. Why can't we do the same here? Binbuf + binshare are great examples actually, because they are exactly the same. The difference is that they were a few small APIs added to support a specific request by discomfitor and were added and *used* in the efl long before any release. Not a new thing I added with the claim "it'll be useful in the future, please get it in before the release". Changes to evas's text rendering: not the same. Extending and refactoring is not the same as introducing completely new features. The former are actually used (by definition) the latter are not. > > Oh dear. I give up. Yes, I'm not perfect, and I don't believe in this. > That's even why I marked the code as "TODO". > Enough comments about it above. > I'd like it to be tested more, but if you're such perfectionist that > can't get along with it... remove. No hard feelings. But at least I > expect the same treat to every other code getting in, fairness. I equally bash newly added elementary widgets all the time, what's more fair than that? As for changes/refactors/extensions to existing code: as I said, it's not the same, the uses are less speculative. > Ok, remove it then. I'm glad we agree. > Could you name the so called API? Why not be specific? Then we can > discuss more specifically what to do: For instance we could 1) remove > the API; 2) mark it as unstable; 3) fix it. Many options, but right > now we're being too vague. Because I'm tired of repeating myself. One thing I remember from the top of my head: I hate the way new classes (esp. inherited classes) are initialized. But again, it's not about the code and API I do know and don't like, it's about the API I (and others) don't know. > > > > I'm not mind guesser... in my mind everything that was discussed in > private was cleared. The comment got in SVN to mark it as 'needs > implementing' with the suggested way. Are there any other flaws? Yes, but as I said, it's not reviewed enough to even know the flaws and if there are flaws. It's a new 5548 lines 122 APIs huge blob of code. Obviously it's not even slightly reviewed. Especially because the lack of example and real use-cases, but not only. > I fail to understand why are you being so picky about this sole piece > of code. There are lots of code introduced, rewritten... yet this is > the sole piece of code that is being bashed. Because all of I said above. > > The way you put it looks like utterly crap and broken in unfixable > ways forever. Given that I've even looked at the API again, from a > fresh view after weeks without reading its code. Still looks good. > Really, why being so harsh with this specific code? As I said above, it's not utter crap or broken in unfixable way forever, but it might be. We can't know without reviewing. There are parts I don't like about it, but those parts are not the important ones, the important ones are the parts I don't know. I'm the only one who kinda reviewed it, and that's it. We need to take a better look at it and actually *use* it in efl to know if that's what we want in. > > Given we're at this situation, I'd request and independent people to > review the code and give us your feedback. I don't want to decrease > the quality of EFL, but at the moment I truly believe I'm not doing > it. If people could be specific on such problems, it would help to > improve or decide to remove it. Yes, it needs more reviews, but the problems are: 1. There's too little time to review it before the release. 2. There aren't enough examples (especially real world examples) for people to comment on. 3. There isn't anything that uses it, so we don't know how the API (and code) handle in real life scenarios. When I reviewed model 2 months ago, we found some issues with it, and you changed the API accordingly, what makes you think this will not happen again? And as I said, not having real use-cases makes it very likely. -- Tom. ------------------------------------------------------------------------------ Better than sec? Nothing is better than sec when it comes to monitoring Big Data applications. Try Boundary one-second resolution app monitoring today. Free. http://p.sf.net/sfu/Boundary-dev2dev _______________________________________________ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
