Re: CAY-400
On Jun 15, 2009, at 4:52 PM, jackCHEN wrote: I think that the key is needed inside the property. Function of property may be extended in future base on the key. Hi Jack, This is a possibility, although I can't think of any meaningful examples right now. During code generation (the main use of properties) the key will be still available - from the MapProperty> attached to the Element. But until we really need it, I am in favor of the "less is better" approach. Cheers, Andrus
Re: CAY-400
2009/6/15 Andrus Adamchik > > On Jun 15, 2009, at 11:49 AM, Aristedes Maniatis wrote: > >> I take your point, but I still think "DataMapElement" is more descriptive. >> Every class in Java is an Object, so using that word as part of the name >> conveys nothing, except that it is sort of a generic multi-purpose thing. At >> least DataMapElement is clear and to the point. And there is no reason why >> an Element can be used to describe theDataMap itself. Slightly odd, but not >> too confusing. >> > > MappingElement then? Not tied to any particular class name and has no > implied ownership. > > Shouldn't we have a rigorous class hierarchy so that common code can be >> kept in superclasses where they belong? >> >> I don't have the experience you do in creating java libraries meant to be >> used by lots of people in different ways, but I can't see how moving to >> interfaces for this one thing will help. If interfaces are to be used, then >> perhaps the ultimate goal is that every 'data map element' should be only an >> interface, allowing a user to swap in any implementation they want without >> having to inherit from our classes. >> > > I don't insist on using interface here, rather I oppose a common superclass > for .map and .query. Inheritance hierarchies can be tricky - it's way too > easy to mix together unrelated things, based on a single coincidental shared > property (no pun intended with "property API" being developed here). So my > rule of thumb is that when in doubt, don't use a common superclass for the > sake of elusive reuse of a few lines of code. > > In fact we probably don't need a common interface without code reuse: .map > and .query can handle properties separately in their own hierarchies. For > the sake of simplicity initially I would even ignore queries completely, and > concentrate on the mapping elements. > > If not, then I think this half way mixture of inheritance and interfaces is >> confusing. There is already a mixture of the two I find sometimes >> disconcerting. >> > > The current state of Cayenne is a result of evolution, not of a one time > design, so there are some rough edges for sure. > > * DataMapElement.Property inner class... Why is this an inner class? It >> >>> is exposed via public methods to the end users, so let's make it a >>> standalone class. >>> >> >> I thought inner class made sense here. The naming is convenient and >> clearer: "DataMapElement.Property" rather than "DataMapElementProperty" >> shows the fact that it is intrinsically part of DataMapElement and not >> relevant as a "Property" outside of that usage. This is just about naming >> and style, and the decision based on what 'looked' right from the >> perspective of a user seeing this class. You originally wanted just a >> Map of properties rather than a whole class, but I thought >> that this was a way to capture more information in the future: properties >> might have other attributes such as whether they travel to the client in >> ROP. But really, they are just a slightly enhanced Map, so an inner class >> seemed to capture that idea. >> > > This is just a matter of API style consistency - in Cayenne we do not have > *public* inner classes at all. So I'd vote for > org.apache.cayenne.map.Property. > > > Also if there is a notion of properties order in the >> >>> element (is there?), I guess the property should be stored in the >>> list. Do we care about properties ordering or simply displaying them in >>> an alphabetic order is ok? >>> I'd say alphabetic is ok. Any scenarios when the order might be >>> important? >>> >> >> I don't think order is important, so the CM can display it in whatever >> order makes sense. >> > > If order is not important, and we don't have to store properties in a list > as a result, we may not even need to store Property.key inside the property. > To me a key is what a holder decided to name the property, not something > that a property itself knows about. I think that the key is needed inside the property. Function of property may be extended in future base on the key. Now, the properties in each DataMapElement/MappingElement are stored using SortedMap, just like Entity using SortedMap to store its attributes and relationships. *Also since we have lots of DataMapElements in the DataMap, and only some will have properties, let's use lazy initialization of the map to save some memory.* You are right, it should be modified to lazy initialization. Andrus
Re: CAY-400
2009/6/15 Andrus Adamchik > > On Jun 15, 2009, at 12:34 PM, jackCHEN wrote: > > * DataMap: private List propertyKeyList - I don't understand >>> this one, and it is encoded in the XML. Before we add that to the schema, could you please explain why it should be there? >>> I think I see what Jack might have done there. That looks like it was >>> used >>> for the CM and should be removed. I'll take a look before it is >>> committed. >>> >> >> >> It is userd for the CM. New added "PropertyPanel" in CM needs a Property >> Key >> List of the DataMap when "PropertyPanel" is initialized. Before discuss >> the >> CM, ti should be removed. The old patch had no this change. >> > > Great. I guess the new events can be removed as well for now. That's right. "DataMapElementEvent" and "DataMapElementListener" can be removed for now.Both support "PropertyPanel" in CM. > > Andrus > >
Re: CAY-400
On Jun 15, 2009, at 12:34 PM, jackCHEN wrote: * DataMap: private List propertyKeyList - I don't understand this one, and it is encoded in the XML. Before we add that to the schema, could you please explain why it should be there? I think I see what Jack might have done there. That looks like it was used for the CM and should be removed. I'll take a look before it is committed. It is userd for the CM. New added "PropertyPanel" in CM needs a Property Key List of the DataMap when "PropertyPanel" is initialized. Before discuss the CM, ti should be removed. The old patch had no this change. Great. I guess the new events can be removed as well for now. Andrus
Re: CAY-400
On Jun 15, 2009, at 11:49 AM, Aristedes Maniatis wrote: I take your point, but I still think "DataMapElement" is more descriptive. Every class in Java is an Object, so using that word as part of the name conveys nothing, except that it is sort of a generic multi-purpose thing. At least DataMapElement is clear and to the point. And there is no reason why an Element can be used to describe theDataMap itself. Slightly odd, but not too confusing. MappingElement then? Not tied to any particular class name and has no implied ownership. Shouldn't we have a rigorous class hierarchy so that common code can be kept in superclasses where they belong? I don't have the experience you do in creating java libraries meant to be used by lots of people in different ways, but I can't see how moving to interfaces for this one thing will help. If interfaces are to be used, then perhaps the ultimate goal is that every 'data map element' should be only an interface, allowing a user to swap in any implementation they want without having to inherit from our classes. I don't insist on using interface here, rather I oppose a common superclass for .map and .query. Inheritance hierarchies can be tricky - it's way too easy to mix together unrelated things, based on a single coincidental shared property (no pun intended with "property API" being developed here). So my rule of thumb is that when in doubt, don't use a common superclass for the sake of elusive reuse of a few lines of code. In fact we probably don't need a common interface without code reuse: .map and .query can handle properties separately in their own hierarchies. For the sake of simplicity initially I would even ignore queries completely, and concentrate on the mapping elements. If not, then I think this half way mixture of inheritance and interfaces is confusing. There is already a mixture of the two I find sometimes disconcerting. The current state of Cayenne is a result of evolution, not of a one time design, so there are some rough edges for sure. * DataMapElement.Property inner class... Why is this an inner class? It is exposed via public methods to the end users, so let's make it a standalone class. I thought inner class made sense here. The naming is convenient and clearer: "DataMapElement.Property" rather than "DataMapElementProperty" shows the fact that it is intrinsically part of DataMapElement and not relevant as a "Property" outside of that usage. This is just about naming and style, and the decision based on what 'looked' right from the perspective of a user seeing this class. You originally wanted just a Map of properties rather than a whole class, but I thought that this was a way to capture more information in the future: properties might have other attributes such as whether they travel to the client in ROP. But really, they are just a slightly enhanced Map, so an inner class seemed to capture that idea. This is just a matter of API style consistency - in Cayenne we do not have *public* inner classes at all. So I'd vote for org.apache.cayenne.map.Property. Also if there is a notion of properties order in the element (is there?), I guess the property should be stored in the list. Do we care about properties ordering or simply displaying them in an alphabetic order is ok? I'd say alphabetic is ok. Any scenarios when the order might be important? I don't think order is important, so the CM can display it in whatever order makes sense. If order is not important, and we don't have to store properties in a list as a result, we may not even need to store Property.key inside the property. To me a key is what a holder decided to name the property, not something that a property itself knows about. Andrus
Re: CAY-400
Hi Ari,Andrus: 2009/6/15 Aristedes Maniatis > On 15/6/09 6:12 PM, Andrus Adamchik wrote: > >> >> * DataMapElement ... While the name itself sounds ok, this implies that >> the DataMap itself can't be a DataMapElement. In 3.0 we do have some >> simple code generation capabilities for DataMap, so adding properties to >> the DataMap seems appropriate. So maybe come back to the >> idea of MappingObject? (also see the next point). >> > > I take your point, but I still think "DataMapElement" is more descriptive. > Every class in Java is an Object, so using that word as part of the name > conveys nothing, except that it is sort of a generic multi-purpose thing. At > least DataMapElement is clear and to the point. And there is no reason why > an Element can be used to describe theDataMap itself. Slightly odd, but not > too confusing. > > > > * Making AbstractQuery extend from DataMapElement may or may not be ok, >> but note that not all queries that can be mapped extend AbstractQuery. >> > > Ah, why not? Shouldn't we have a rigorous class hierarchy so that common > code can be kept in superclasses where they belong? > > > In fact I'd like to get rid of this inheritance going forward... So >> looks like the whole DataMapElement/MappingObject should >> be an interface anyways. >> > > I don't have the experience you do in creating java libraries meant to be > used by lots of people in different ways, but I can't see how moving to > interfaces for this one thing will help. If interfaces are to be used, then > perhaps the ultimate goal is that every 'data map element' should be only an > interface, allowing a user to swap in any implementation they want without > having to inherit from our classes. If not, then I think this half way > mixture of inheritance and interfaces is confusing. There is already a > mixture of the two I find sometimes disconcerting. > > > > * DataMap: private List propertyKeyList - I don't understand >> this one, and it is encoded in the XML. Before we add that to the >> schema, could you please explain why it should be there? >> > > I think I see what Jack might have done there. That looks like it was used > for the CM and should be removed. I'll take a look before it is committed. It is userd for the CM. New added "PropertyPanel" in CM needs a Property Key List of the DataMap when "PropertyPanel" is initialized. Before discuss the CM, ti should be removed. The old patch had no this change. > > > > * Patch for the schema includes the entire file. It is not clear what >> was changed there. >> > > Most of it :-). The indenting changed because we used XSD inheritance to > describe the way the property can be attached to the parent class, matching > the Java hierarchy. > > > * DataMapElement.Property inner class... Why is this an inner class? It >> is exposed via public methods to the end users, so let's make it a >> standalone class. >> > > I thought inner class made sense here. The naming is convenient and > clearer: "DataMapElement.Property" rather than "DataMapElementProperty" > shows the fact that it is intrinsically part of DataMapElement and not > relevant as a "Property" outside of that usage. This is just about naming > and style, and the decision based on what 'looked' right from the > perspective of a user seeing this class. You originally wanted just a > Map of properties rather than a whole class, but I thought > that this was a way to capture more information in the future: properties > might have other attributes such as whether they travel to the client in > ROP. But really, they are just a slightly enhanced Map, so an inner class > seemed to capture that idea. > > > Also if there is a notion of properties order in the >> element (is there?), I guess the property should be stored in the >> list. Do we care about properties ordering or simply displaying them in an >> alphabetic order is ok? >> I'd say alphabetic is ok. Any scenarios when the order might be important? >> > > I don't think order is important, so the CM can display it in whatever > order makes sense. > > > * Property.getHolder() is not used anywhere. Let's not add API we don't >> need. Also since we have lots of DataMapElements in the DataMap, and >> only some will have properties, let's use lazy initialization of >> the map to save some memory. >> > > Sure. > > I also have some notes on the Modeler, but I suggest we settle on the >> core framework approach first, and do incremental patches. It is much >> easier to review and discuss things in small manageable pieces. >> > > Fair enough. If Jack is still around he might like to do the tidying up, > otherwise I'll look after it. > > > Ari Maniatis >
Re: CAY-400
On 15/6/09 6:12 PM, Andrus Adamchik wrote: * DataMapElement ... While the name itself sounds ok, this implies that the DataMap itself can't be a DataMapElement. In 3.0 we do have some simple code generation capabilities for DataMap, so adding properties to the DataMap seems appropriate. So maybe come back to the idea of MappingObject? (also see the next point). I take your point, but I still think "DataMapElement" is more descriptive. Every class in Java is an Object, so using that word as part of the name conveys nothing, except that it is sort of a generic multi-purpose thing. At least DataMapElement is clear and to the point. And there is no reason why an Element can be used to describe theDataMap itself. Slightly odd, but not too confusing. * Making AbstractQuery extend from DataMapElement may or may not be ok, but note that not all queries that can be mapped extend AbstractQuery. Ah, why not? Shouldn't we have a rigorous class hierarchy so that common code can be kept in superclasses where they belong? In fact I'd like to get rid of this inheritance going forward... So looks like the whole DataMapElement/MappingObject should be an interface anyways. I don't have the experience you do in creating java libraries meant to be used by lots of people in different ways, but I can't see how moving to interfaces for this one thing will help. If interfaces are to be used, then perhaps the ultimate goal is that every 'data map element' should be only an interface, allowing a user to swap in any implementation they want without having to inherit from our classes. If not, then I think this half way mixture of inheritance and interfaces is confusing. There is already a mixture of the two I find sometimes disconcerting. * DataMap: private List propertyKeyList - I don't understand this one, and it is encoded in the XML. Before we add that to the schema, could you please explain why it should be there? I think I see what Jack might have done there. That looks like it was used for the CM and should be removed. I'll take a look before it is committed. * Patch for the schema includes the entire file. It is not clear what was changed there. Most of it :-). The indenting changed because we used XSD inheritance to describe the way the property can be attached to the parent class, matching the Java hierarchy. * DataMapElement.Property inner class... Why is this an inner class? It is exposed via public methods to the end users, so let's make it a standalone class. I thought inner class made sense here. The naming is convenient and clearer: "DataMapElement.Property" rather than "DataMapElementProperty" shows the fact that it is intrinsically part of DataMapElement and not relevant as a "Property" outside of that usage. This is just about naming and style, and the decision based on what 'looked' right from the perspective of a user seeing this class. You originally wanted just a Map of properties rather than a whole class, but I thought that this was a way to capture more information in the future: properties might have other attributes such as whether they travel to the client in ROP. But really, they are just a slightly enhanced Map, so an inner class seemed to capture that idea. Also if there is a notion of properties order in the element (is there?), I guess the property should be stored in the list. Do we care about properties ordering or simply displaying them in an alphabetic order is ok? I'd say alphabetic is ok. Any scenarios when the order might be important? I don't think order is important, so the CM can display it in whatever order makes sense. * Property.getHolder() is not used anywhere. Let's not add API we don't need. Also since we have lots of DataMapElements in the DataMap, and only some will have properties, let's use lazy initialization of the map to save some memory. Sure. I also have some notes on the Modeler, but I suggest we settle on the core framework approach first, and do incremental patches. It is much easier to review and discuss things in small manageable pieces. Fair enough. If Jack is still around he might like to do the tidying up, otherwise I'll look after it. Ari Maniatis
Re: CAY-400
On Jun 15, 2009, at 11:12 AM, Andrus Adamchik wrote: Also if there is a notion of properties order in the element (is there?), I guess the property should be stored in the list. If there's no notion of Sorry... didn't finish this sentence. This was more of a question - do we care about properties ordering or simply displaying them in an alphabetic order is ok? I'd say alphabetic is ok. Any scenarios when the order might be important? Andrus
Re: CAY-400
Hi Jack, Ari Finally had some time to look at the patch. It is moving in the right direction, but there are still some issues we need to address: * DataMapElement ... While the name itself sounds ok, this implies that the DataMap itself can't be a DataMapElement. In 3.0 we do have some simple code generation capabilities for DataMap, so adding properties to the DataMap seems appropriate. So maybe come back to the idea of MappingObject? (also see the next point). * Making AbstractQuery extend from DataMapElement may or may not be ok, but note that not all queries that can be mapped extend AbstractQuery. In fact I'd like to get rid of this inheritance going forward... So looks like the whole DataMapElement/MappingObject should be an interface anyways. * DataMap: private List propertyKeyList - I don't understand this one, and it is encoded in the XML. Before we add that to the schema, could you please explain why it should be there? * Patch for the schema includes the entire file. It is not clear what was changed there. * DataMapElement.Property inner class... Why is this an inner class? It is exposed via public methods to the end users, so let's make it a standalone class. Also if there is a notion of properties order in the element (is there?), I guess the property should be stored in the list. If there's no notion of * Property.getHolder() is not used anywhere. Let's not add API we don't need. Also since we have lots of DataMapElements in the DataMap, and only some will have properties, let's use lazy initialization of the map to save some memory. I also have some notes on the Modeler, but I suggest we settle on the core framework approach first, and do incremental patches. It is much easier to review and discuss things in small manageable pieces. Andrus On Jun 5, 2009, at 5:48 PM, Andrus Adamchik wrote: [Let's take this to a separate thread.] On Jun 5, 2009, at 5:33 PM, Aristedes Maniatis wrote: Back on dev topics, Jack has finished his secondment at ish now and posted his DataMapElement.Property implementation. I'll do some cleanup on it (mostly naming and style) and some commenting and commit it unless anyone has something to say. Could you please let it sit in Jira for some time. I'd like to review it, but can't do it right this second. Andrus
