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
