search

OpenFn / core

The central job processing module used in OpenFn v1.
https://www.openfn.org/
GNU General Public License v3.0
18 stars 8 forks source link

Adding a new execute function without command line interface #15

Closed magp18 closed 3 years ago

magp18 commented 4 years ago

Thank you for the prompt reply and suggestions! I will try to implement them by today.

magp18 commented 4 years ago

After reading your feedback, I feel like you are right, using an adaptor might not be necessary in the core module. I will keep the test analog to the test already provided from you and remove the language-dhis2 dependency there so that it does not fail when dhis2 is not there.

stuartc commented 4 years ago

Thanks for updating, @magp18 . I had a quick chat with @taylordowns2000 , and still two things (at least) are outstanding before we can get this merged.

  1. We can't move forward with the tests failing. The concept seems cool, but the PR should pass the tests on Travis

  2. A little more context seems important. Is this part of the SwissTPH Nigeria project? How will this functionality benefit that implementation? (A link to the repo that’s using this would really help so we can see how it’s being used.)
    Could you update the problem the PR description detailing the problem and solution.

  3. I think we may be missing an important refactoring step, your changes have made me rethink the implementation in the cli - while it made sense at the time to have all the 'setup' stuff all in that file; it doesn't make sense to have it that way anymore.
    I propose we move the setup phases into their own functions, that way when using Execute from a script you have all the bits to set up the arguments at hand, I think that would be better than copy pasting the setup phase into a new function.
    This will help maintain parity between the CLI and programmatic use.

Apologies for being a little late to the party, @taylordowns2000 I'm happy to chip in my code suggestions and check in with @magp18 if it satisfies his expectations. Thoughts?

magp18 commented 4 years ago

Hi @stuartc thank you for your feedback. Thank you for your time.

  1. I thought after the last commit, it would work. I took out the dependency on the dhis2 language so that it should also work without having it installed. I just realized that I have access to the travis output and can actually see what the error was. After checking, I guess it is because I assume the http language is installed to have the execute function.

  2. I would say that it is not for a specific problem but rather to complement the functionality with a pure javascript function so that it is easier to create javascript projects using your functionality that we would then use to create a mediator together with openhim and openfn while programmatically managing the files e.g. expression. Everything done so far is in this repo

  3. I think I know what you mean. It is also the reason why my code is failing right? Because you guys have execute isolated from the wrapping which makes use of execute provided in the language-common package, while I have it one file which is the reason why it fails, as it requires the execute function from the language to work (which for me works because I have it installed). I guess I could split them too, then I would have an alternative to the CLI file using only javascript instead of an alternative to the execute function, is this your suggestion?

Other than this, I am confident that adding: 

install:
- npm install https://github.com/OpenFn/language-common.git

to the travis.yml file would solve the issue. But I do agree that separating the preparation and the execution parts (if I understood you well) make it cleaner so I will work on that too.

stuartc commented 3 years ago

Hey @magp18 - I've just taken big swing at refactoring a whole bunch of stuff to help you in the quest to run expressions from Node (instead of via the CLI). I'd like to thank you for your collaboration!

I felt that your requirement should be a first-class citizen in the library - some I've abstracted and brushed up a whole bunch of things. While I'm certain the solutions isn't as simple as yours I think it may be important to be able to control exactly what goes on.

If you would like to take a look at what I'm proposing please check out #16 .

taylordowns2000 commented 3 years ago

@magp18 , hope you've been well! We're going to move forward with this for now, but I'd really love to hear if it solves for your use case as intended. I've merged #16 into master and look forward to your feedback :-)

magp18 commented 3 years ago

Hi both! I am well, I hope you have been well too! I am sorry for the delay, I was busy finalizing and presenting the thesis. Thank you very much for introducing the programmatical approach! It seems to be very intuitive to use, I will talk to my supervisor to implement it so that we can tell you how it is. Again, thank you for implementing this and for your collaboration!

stuartc commented 3 years ago

You're absolutely welcome, we would love your feedback! I know it's not quite as 'single method' as your original proposal, but I think with some example use cases we provide some composed functions that close the gap further between the current programmatic example and a single function.

On Thu, 04 Feb 2021, 17:56 Marco Pereira notifications@github.com wrote:

Hi both! I am well, I hope you have been well too! I am sorry for the delay, I was busy finalizing and presenting the thesis. Thank you very much for introducing the programmatical approach! It seems to be very intuitive to use, I will talk to my supervisor to implement it so that we can tell you how it is. Again, thank you for implementing this and for your collaboration!

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/OpenFn/core/pull/15#issuecomment-773413022, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABDLHBUCI6DR3Q4PRHJZTDS5K7TXANCNFSM4RUI4UKQ .