Re: [Zorba-coders] [Merge] lp:~zorba-coders/zorba/feature-cloudant into lp:zorba
- The indentation has several problems. Could you please make it consistent. Especially in the C++ code. sure. - Why don't you use the same approach as we did for xbrl-cloud and keep the connection map in JSONiq (using the map module)? I think that would be more appropriate for this module than replicating 400 lines of c++ code. If this is a recurring pattern, we might think about providing core support for it but I feel that the JSONiq map is good enough for now. Also, I wouldn't worry about the disconnect at this point. When the story was discussed I was suggested to manage connections as the JDBC module does. But I probably misunderstood. I updated the code to manage connection from XQuery and removed all c++ code. -- https://code.launchpad.net/~zorba-coders/zorba/feature-cloudant/+merge/177111 Your team Zorba Coders is subscribed to branch lp:zorba. -- Mailing list: https://launchpad.net/~zorba-coders Post to : zorba-coders@lists.launchpad.net Unsubscribe : https://launchpad.net/~zorba-coders More help : https://help.launchpad.net/ListHelp
Re: [Zorba-coders] [Merge] lp:~zorba-coders/zorba/feature-cloudant into lp:zorba
Review: Resubmit External module merge proposal in lp:~zorba-coders/zorba/feature-external-cloudant-module and lp:~zorba-coders/zorba/cloudant-module -- https://code.launchpad.net/~zorba-coders/zorba/feature-cloudant/+merge/177111 Your team Zorba Coders is subscribed to branch lp:zorba. -- Mailing list: https://launchpad.net/~zorba-coders Post to : zorba-coders@lists.launchpad.net Unsubscribe : https://launchpad.net/~zorba-coders More help : https://help.launchpad.net/ListHelp
Re: [Zorba-coders] [Merge] lp:~zorba-coders/zorba/feature-cloudant into lp:zorba
- The indentation has several problems. Could you please make it consistent. Especially in the C++ code. - Why don't you use the same approach as we did for xbrl-cloud and keep the connection map in JSONiq (using the map module)? I think that would be more appropriate for this module than replicating 400 lines of c++ code. If this is a recurring pattern, we might think about providing core support for it but I feel that the JSONiq map is good enough for now. Also, I wouldn't worry about the disconnect at this point. -- https://code.launchpad.net/~zorba-coders/zorba/feature-cloudant/+merge/177111 Your team Zorba Coders is subscribed to branch lp:zorba. -- Mailing list: https://launchpad.net/~zorba-coders Post to : zorba-coders@lists.launchpad.net Unsubscribe : https://launchpad.net/~zorba-coders More help : https://help.launchpad.net/ListHelp
Re: [Zorba-coders] [Merge] lp:~zorba-coders/zorba/feature-cloudant into lp:zorba
I did not find a description of the process necessary to propose a new module in the wiki. What should I do? Thanks -- https://code.launchpad.net/~zorba-coders/zorba/feature-cloudant/+merge/177111 Your team Zorba Coders is subscribed to branch lp:zorba. -- Mailing list: https://launchpad.net/~zorba-coders Post to : zorba-coders@lists.launchpad.net Unsubscribe : https://launchpad.net/~zorba-coders More help : https://help.launchpad.net/ListHelp
Re: [Zorba-coders] [Merge] lp:~zorba-coders/zorba/feature-cloudant into lp:zorba
There isn't really a fixed process for that since it happens so rarely. Here's what I'd suggest: 1. Create the module as a stand-alone directory, so that it works when located in the zorba_modules directory as part of a Zorba build. 2. Push that module to Launchpad as lp:~zorba-coders/zorba/cloudant-module . 3. I will create a Launchpad series for that branch, so it has the standard lp:zorba/cloudant-module name. 4. Propose a change to the Zorba trunk that adds lp:zorba/cloudant-module to modules/ExternalModules.conf . 5. People can place code reviews for the module on that merge proposal. Once that's merged, in future it should be possible propose subsequent changes to the Cloudant module as merge proposals directly to lp:zorba/cloudant-module, as normal. It's just the bootstrapping procedure that needs be a bit manual. -- https://code.launchpad.net/~zorba-coders/zorba/feature-cloudant/+merge/177111 Your team Zorba Coders is subscribed to branch lp:zorba. -- Mailing list: https://launchpad.net/~zorba-coders Post to : zorba-coders@lists.launchpad.net Unsubscribe : https://launchpad.net/~zorba-coders More help : https://help.launchpad.net/ListHelp
Re: [Zorba-coders] [Merge] lp:~zorba-coders/zorba/feature-cloudant into lp:zorba
Review: Needs Information The module itself looks OK at a quick glance. However, unless I'm missing something, I don't think this needs to be a core module. It should be a non-core module in a separate branch. -- https://code.launchpad.net/~zorba-coders/zorba/feature-cloudant/+merge/177111 Your team Zorba Coders is subscribed to branch lp:zorba. -- Mailing list: https://launchpad.net/~zorba-coders Post to : zorba-coders@lists.launchpad.net Unsubscribe : https://launchpad.net/~zorba-coders More help : https://help.launchpad.net/ListHelp
Re: [Zorba-coders] [Merge] lp:~zorba-coders/zorba/feature-cloudant into lp:zorba
Given the comments I received for the http-module I fear that the way I am disconnecting from cloudant is not correct, because curl_global_init/cleanup must not be called multiple times from different modules. I tried disconnecting invoking an XQuery function but this does not work because the static context is already destroyed. I left it commented in case my code was just wrong. If disconnecting via XQuery is not possible, i guess we can: 1) avoid loggin in. There is another way of communicating with cloudant where each request contains user and password. 2) not disconnect. 3) share the singleton class which manages curl for the http client Do you have another idea or a hint? Thanks -- https://code.launchpad.net/~zorba-coders/zorba/feature-cloudant/+merge/177111 Your team Zorba Coders is subscribed to branch lp:zorba. -- Mailing list: https://launchpad.net/~zorba-coders Post to : zorba-coders@lists.launchpad.net Unsubscribe : https://launchpad.net/~zorba-coders More help : https://help.launchpad.net/ListHelp
Re: [Zorba-coders] [Merge] lp:~zorba-coders/zorba/feature-cloudant into lp:zorba
Review: Needs Fixing Actually, a few code problems: 1. ConnectionMap::destroy() invokes a function in the http://28msec.io/modules/cloudant namespace. I think that should be in http://zorba.io/modules/cloudant, right? 2. The files in cloudant.xq.src have 28msec copyright notices. 3. I think the changes in modules/CMakeLists.txt are wrong; why are you eliminating the http-client subdirectory, for instance? Also, the search for CURL needs to only be once; will you remove the FIND_PACKAGE(CURL) from the http-client subdirectory, once those modules are all merged? -- https://code.launchpad.net/~zorba-coders/zorba/feature-cloudant/+merge/177111 Your team Zorba Coders is subscribed to branch lp:zorba. -- Mailing list: https://launchpad.net/~zorba-coders Post to : zorba-coders@lists.launchpad.net Unsubscribe : https://launchpad.net/~zorba-coders More help : https://help.launchpad.net/ListHelp
Re: [Zorba-coders] [Merge] lp:~zorba-coders/zorba/feature-cloudant into lp:zorba
Ahh, I see, that call in ConnectionMap::destroy() is commented out. Didn't notice that the first time. Regarding the curl disconnect problems: That would all go away if this module used the http-client module, rather than using C++ to invoke CURL directly. Is there a reason this module couldn't be written in pure XQuery? Dana's original goal for Zorba was the the built-in http-client module would be the single way to access HTTP, and all uses would go through that. We're not perfect yet, but we're getting better. -- https://code.launchpad.net/~zorba-coders/zorba/feature-cloudant/+merge/177111 Your team Zorba Coders is subscribed to branch lp:zorba. -- Mailing list: https://launchpad.net/~zorba-coders Post to : zorba-coders@lists.launchpad.net Unsubscribe : https://launchpad.net/~zorba-coders More help : https://help.launchpad.net/ListHelp
Re: [Zorba-coders] [Merge] lp:~zorba-coders/zorba/feature-cloudant into lp:zorba
Hi Chris, thanks for the quick review. I think it is currently a core module due to the auto-disconnect functionality. External module can have cpp external functions? However, as I wrote in my previous comment, this feature is almost certainly wrong at the moment. If I recall correctly, Matthias suggested me to make the module autodisconnect similarly to the way the JDBC module does. So I am not sure if my implementation is wrong but the autodisconnect feature possible. Thanks for the comments -- https://code.launchpad.net/~zorba-coders/zorba/feature-cloudant/+merge/177111 Your team Zorba Coders is subscribed to branch lp:zorba. -- Mailing list: https://launchpad.net/~zorba-coders Post to : zorba-coders@lists.launchpad.net Unsubscribe : https://launchpad.net/~zorba-coders More help : https://help.launchpad.net/ListHelp
Re: [Zorba-coders] [Merge] lp:~zorba-coders/zorba/feature-cloudant into lp:zorba
Actually, a few code problems: 1. ConnectionMap::destroy() invokes a function in the http://28msec.io/modules/cloudant namespace. I think that should be in http://zorba.io/modules/cloudant, right? This is commented out :) I tried doing it in XQuery but I didn't manage so I left the code commented to get a feedback if the code was just wrong or using the static context to call a XQuery function when the connectionmap is destroyed is just impossible. 2. The files in cloudant.xq.src have 28msec copyright notices. Thanks. I didn't update them all apparently. 3. I think the changes in modules/CMakeLists.txt are wrong; why are you eliminating the http-client subdirectory, for instance? Also, the search for CURL needs to only be once; will you remove the FIND_PACKAGE(CURL) from the http-client subdirectory, once those modules are all merged? Since in its current state this module depends on CURL i made a single search for the CURL library and moved both the ADD_DIRECTORY(http-client) and ADD..(cloudant) inside the same check. The http client cmakelist doesn't do the search again. At least this was my intention, I asked Matthias offline if this make sense. However, since I am not sure of the autodisconnect approach anymore this change may need to be reverted. I asked Matthias via mail if that mak To make a single FIND_PACKAGE(CURL) -- https://code.launchpad.net/~zorba-coders/zorba/feature-cloudant/+merge/177111 Your team Zorba Coders is subscribed to branch lp:zorba. -- Mailing list: https://launchpad.net/~zorba-coders Post to : zorba-coders@lists.launchpad.net Unsubscribe : https://launchpad.net/~zorba-coders More help : https://help.launchpad.net/ListHelp
Re: [Zorba-coders] [Merge] lp:~zorba-coders/zorba/feature-cloudant into lp:zorba
Ahh, I see, that call in ConnectionMap::destroy() is commented out. Didn't notice that the first time. Regarding the curl disconnect problems: That would all go away if this module used the http-client module, rather than using C++ to invoke CURL directly. Is there a reason this module couldn't be written in pure XQuery? Dana's original goal for Zorba was the the built-in http-client module would be the single way to access HTTP, and all uses would go through that. We're not perfect yet, but we're getting better. I tried doing it in XQuery but I didn't manage so I left the code commented to get a feedback if the code was just wrong or using the static context to call a XQuery function when the connectionmap is destroyed is just impossible. -- https://code.launchpad.net/~zorba-coders/zorba/feature-cloudant/+merge/177111 Your team Zorba Coders is subscribed to branch lp:zorba. -- Mailing list: https://launchpad.net/~zorba-coders Post to : zorba-coders@lists.launchpad.net Unsubscribe : https://launchpad.net/~zorba-coders More help : https://help.launchpad.net/ListHelp
Re: [Zorba-coders] [Merge] lp:~zorba-coders/zorba/feature-cloudant into lp:zorba
Yes, non-core modules can have C++ external functions; take a look at the process module, for instance. -- https://code.launchpad.net/~zorba-coders/zorba/feature-cloudant/+merge/177111 Your team Zorba Coders is subscribed to branch lp:zorba. -- Mailing list: https://launchpad.net/~zorba-coders Post to : zorba-coders@lists.launchpad.net Unsubscribe : https://launchpad.net/~zorba-coders More help : https://help.launchpad.net/ListHelp
Re: [Zorba-coders] [Merge] lp:~zorba-coders/zorba/feature-cloudant into lp:zorba
Review: Needs Information Since in its current state this module depends on CURL i made a single search for the CURL library and moved both the ADD_DIRECTORY(http-client) and ADD..(cloudant) inside the same check. The http client cmakelist doesn't do the search again. Aha! Now I see what you did. Ok, that's fine and correct, yes. So, assuming you fix the copyright notices, my Needs Fixing comments are taken care of. However, I still don't think this should be a core module, so I'm leaving my vote at Needs Information. -- https://code.launchpad.net/~zorba-coders/zorba/feature-cloudant/+merge/177111 Your team Zorba Coders is subscribed to branch lp:zorba. -- Mailing list: https://launchpad.net/~zorba-coders Post to : zorba-coders@lists.launchpad.net Unsubscribe : https://launchpad.net/~zorba-coders More help : https://help.launchpad.net/ListHelp
Re: [Zorba-coders] [Merge] lp:~zorba-coders/zorba/feature-cloudant into lp:zorba
I agree that an external module is best suited for this module. I will look in the wiki to see the submit procedure. -- https://code.launchpad.net/~zorba-coders/zorba/feature-cloudant/+merge/177111 Your team Zorba Coders is subscribed to branch lp:zorba. -- Mailing list: https://launchpad.net/~zorba-coders Post to : zorba-coders@lists.launchpad.net Unsubscribe : https://launchpad.net/~zorba-coders More help : https://help.launchpad.net/ListHelp
