Scott Gray wrote: > On 18/03/2010, at 11:04 AM, Adam Heath wrote: > >> Scott Gray wrote: >>> On 18/03/2010, at 10:50 AM, Adam Heath wrote: >>> >>>> Jacopo Cappellato wrote: >>>>> Adam, >>>>> >>>>> this is going to be a long thread and I don't want it to be. >>>>> I already tried to explain why I decided to change the primary key of >>>>> OrderShipment without following the deprecation mechanism (that I think >>>>> it is a very good best practice) in this specific situation. >>>>> I perfectly know when and how the shipment plans (i.e. the feature for >>>>> which the OrderShipment entity was introduced) were implemented: I know >>>>> because I contributed it (and the OrderShipment entity of course). >>>>> I also know that it was only partially implemented and so I would be very >>>>> surprised (especially because no one over the years ever asked or >>>>> reported bugs etc.. about it - apart from me that was aware of its >>>>> existence) if it is really used in production by someone. This is simply >>>>> to explain that I was not irresponsible when I did it. >>>>> >>>>> However you are right if you say that I can't be completely sure if >>>>> someone somewhere is using the entity. Also, lately the community seems >>>>> to be more inclined to transform "best practices" (to be applied "cum >>>>> granu salis") into "policies" to be applied blindly. >>>>> In some way this is good because it requires less skills and creativity >>>>> (but more pragmatism) and also less skilled persons can become good >>>>> reviewers just by following written rules. >>>>> This is probably less interesting but maybe necessary given the grow of >>>>> the community. >>>>> >>>>> So ok, I am convinced now, we have to deprecate that entity etc. >>>>> >>>>> What does your book of policies says about this: is it something I *have* >>>>> to do as a punishment for this bad commit or will you take care of it? >>>> If I implied that it had to be you to do it, I apoligize. I didn't >>>> mean it to sound that way. >>>> >>>> I was trying to discuss with you about the reasons behind this change, >>>> so that we could all agree as to what the reasons and use cases were. >>>> >>>> Now that it seems we are on the same page, we can go about fixing this >>>> as a team. I'll admit, however, that I have never gone thru an entity >>>> deprecation exercise. And OrderShipment is a very low-level entity, >>>> that has tons of references, so doing this right could be very >>>> complex. The reason why I was so heated was that I understood this >>>> would be a lot of work, and I was trying to see if there is a better >>>> way than changing the primary key. >>>> >>>> The reason the original committer is generally the best one to fix >>>> their original commit, is because that original committer knows the >>>> most about what they were trying to do. That original person knows >>>> their own code they wrote, and probably has an understand of the rest >>>> of the files that they were modifying. >>>> >>>> However, due to the scope of this deprecation, we need the rest of the >>>> community to help out. Unless we decide to revert this primary key >>>> change, and due this a different way. >>>> >>>>> PS: BTW, if no one will implement the deprecation pattern in the next few >>>>> days I will do that, no problem. >>>> There's no big hurry. My previously mentioned importer already had >>>> the shipGroupSeqId available(it was hard-coded to 00001, as it's >>>> somewhat simple), so it wasn't a big change for me personally. >>> I'm yet to form an opinion on whether the approach Jacopo took was a valid >>> one, I've been looking at the code and I don't yet understand what >>> problems this change may have caused for downstream users. >>> Adam, would you mind explaining in more detail how the change has effected >>> you negatively? >> A new field was added to the OrderShipment entity, and that field was >> made a primary key. >> >> My importer, which started from an empty database, thru an error, >> because this field was not set, and null is not allowed. As I said, >> the fix for me was simple, as this particular job hasn't gone live >> yet, so I don't have to worry so much about migration. > > I would argue that in this case the early failure was a good thing. If the > deprecation pattern had been followed it's possible you wouldn't have noticed > until much later and then would have had to deal with updating your importer > as well as migrating the data from the old table to the new.
You don't understand how deprecation works. The entity is renamed. So there would still be the failure in my importer, as it tried to update an entity that doesn't exist. Additionally, if someone upgrading their data does an xml dump, the old data will be dumped with the entity named OrderShipment. After installing the newer ofbiz, that identiy will also not exist, so the xml import will fail as well, when it can't find the OrderShipment entity(which was renamed to OldOrderShipment).
