Oh and for the rest of the folks in our community, this is a very critical piece of work, I think more than one pair of eyes should be on it, so please take a look at it if you can.
On Sun, Jul 15, 2018 at 7:47 PM, Taher Alkhateeb <[email protected]> wrote: > Hello Mathieu, > > First of all, I would like to say I really enjoy looking at your code. > I can tell that it was done with lots of love :) > > I took a deeper look at the code and I have a few more questions / > comments / suggestions: > - First of all and to stay consistent with the rest of the OFBiz > architecture, you can perhaps call your controller object > "ControllerConfig" just like we have "ContainerConfig" > "ComponentConfig" and so on. > - What do you have a temporary converter from Map to MultiMap? That > piece of code is really sour on the eyes with all those overrides > - I still don't understand your answer on the new data structure > "MultiValuedMapContext". This is essentially a whole new data > structure designed by you and it overrides nearly all standard > methods. Having such a custom and un-tested data structure might be > worrying. Why aren't you going with a standard and tested solution? > What methods exactly do we need that requires that level of > customization? Is it the getRequestMapMap and getRequestMultiMap? Why > not simply create two custom logic methods in a Util class to handle > that instead of that mesh of object inheritance? I find it a little > worrying to strongly tie core framework classes with external > libraries. We should perhaps attempt to do that in a de-coupled > fashion. For example, take a look at Start.java where it has a > function to parse online arguments. This function completely hides the > libraries away from the framework. Maybe we should attempt something > similar? > - I am assuming from the comment in ConfigXMLReader that you intend to > refactor the converter right? How will this conversion happen, when > and how? > > Thank you for your patience and follow through :) > > On Thu, Jul 12, 2018 at 4:37 PM, Mathieu Lirzin > <[email protected]> wrote: >> Hello, >> >> Taher Alkhateeb <[email protected]> writes: >> >>> Examining your patch I have a few comments / questions: >> >> FYI I have updated the third patch on OFBIZ-10438 [1] to improve the >> tests for the ‘resolveMethod’ static method. >> >> [1] https://issues.apache.org/jira/browse/OFBIZ-10438 >> >> Thanks. >> >> -- >> Mathieu Lirzin >> GPG: F2A3 8D7E EB2B 6640 5761 070D 0ADE E100 9460 4D37
