Return process or PID in IGitResult about dugite HOT 7 CLOSED

I'm not particularly convinced about that specific approach, to be honest, it feels a bit hacky to me 😳

I think this would require a bigger API change:

• Create a new abstraction called, let's say, GitTask, which represents a git process.
• This abstraction would have:
  • An internal private property storing the PID of the git process (or something equivalent).
  • A result: Promise<IGitResult> property to obtain the result of the process.
  • A method like cancel() to kill the process while it's running. I imagine the implementation of this method would kill the process using its PID and somehow signal that the process was cancelled.
• exec would return a GitTask instance instead of Promise<IGitResult>.

With that change, code that now looks like this:

const result = await exec(…)

Would look like this:

const task = exec(…)
const result = await task.result

And you can invoke task.cancel() to kill it at any time. That means IGitResult might need an additional field to indicate when the process was cancelled.

Does that make sense? 😳  I'm open to feedback and other suggestions about this 😄  My main concern is that this breaks the current API… but maybe we can create a new execTask function that returns this GitTask, and then implement the existing exec like this:

public static exec(args: string[], path: string, options?: IGitExecutionOptions,pidCallback?: (pid: number) => void): Promise<IGitResult> {
  const task = execTask(…)
  return task.result
}

@tidy-dev What do you think?

from dugite.

@sergiou87 All makes sense to me and like the bit about making an execTask to reduce implementation impact.

from dugite.

And if this is getting implemented, can I get assigned to this?

from dugite.

Hey @maifeeulasad! 👋

Thank you for your suggestion. However, I think that wouldn't be very useful because IGitResult represents the result of the git process once it's finished. At that point, the PID is not valid because the process doesn't exist anymore 😓

You would need to return the PID along with the Promise<IGitResult>, but that wouldn't end in a very clean API, in my opinion 😞

from dugite.

@sergiou87 can we pass an additional optional callback function, like this and that will return us the PID ?

public static exec(args: string[], path: string, options?: IGitExecutionOptions,pidCallback?: (pid: number) => void): Promise<IGitResult>;

Then we have the pid, check if it is undefined or not, if not, then we can use it to kill or other purposes.
This is what I thought of till now.

from dugite.

@sergiou87 damn, this is so cool
can I take part in the implementation?

from dugite.

Absolutely @maifeeulasad! ❤️

Please, consider adding some tests around that new cancel method to make sure it works as expected before we attempt to use it in GitHub Desktop 😄

Thank you for your work!

from dugite.