danwatford commented on PR #401: URL: https://github.com/apache/ofbiz-framework/pull/401#issuecomment-1350584194
Hi @mbrohl , It certainly wasn't my intention to cause any upset and I am sorry if my actions came across that way. There is a lot of value locked up in the various PRs that have been submitted to OFBiz which cannot be realised until they are merged. I feel bad for authors who put a lot of time and effort into preparing pull requests which, due to pressures on committers' availability, don't see their contributions merged - particularly when the proposed changes seem in line with the project's goals. (In this case the movement away from simplelang to groovy) After sending my initial question to you and @SebastianEcomify , I decided to go ahead with partially applying changes - with commit messages crediting Sebastian - since Inventory Management is an area I am actively working with and thought working through the PR would help with my understanding of the area. I honestly thought you and Sebastian would have been pleased to see someone start working through this PR. If this is not the case please say so, but also please let me know why so that we can void such situations in future. > ... I do not understand why it's easier to split Sebastian's work and create new PR's for it, can you elaborate more where the problem is? The main part of this pull request is a big Groovy script file, porting the implementation more than 30 services from minilang to groovy script. Those services, although notionally related to inventory, implement a variety of functionality. When I review a PR I try to understand the impact of the change and assess its correctness. In this case that means reading the groovy implementation of each ported service and checking it aligns with the original minilang version. While doing this, I need to ensure I understand both the original and new implementation, and also determine whether any discrepancies are a cause for concern or whether they are just an artifact of two different language approaches. Case in point: The groovy implementation of updateOldInventoryToDetailSingle sets some entity attributes to null, whereas the original minilang set them to the empty string "". Now there may be no effective difference between approaches, but I still needed to check. (In this case, that checking resulted in OFBIZ-12723 and meant we could remove a bit of code). That checking process should speed up as I become more familiar with minilang. Next, for each ported service, I need to understand where it is used in OFBiz and identify a way to test that the new implementation works. Since I am not yet familiar with inventory management, this takes a lot of time. To me, the above takes a lot of work and is difficult to track and communicate progress. Others might find it easy. We are all different. If I stick with reviewing the original PR, then a lot of my time, as well as the time Sebastian originally spent authoring the PR, won't be realised as value until the entire work is completed, and this probably wouldn't get done. To make the task of reviewing this PR's changes more management, I decided to try and identify smaller pieces that could be handled as separate PRs, ensuring credit is given to Sebastian for those changes. In summary, smaller patches are easier to review and test, and less risky to merge. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
