applyPatches should fail if can't apply a patch

[piranna]

On applyPatches, if a patch can't be applied it's just notified to the user. This should call inmediatly to completed() and raise the full application of patches as a failure.

[piranna]

I've fixed this on issue #130.

[kpdecker]

I disagree. The user should decide how they want to handle this. They may want to have everything possible apply and be notified about what did not.

[piranna]

I was thinking about this posibility too, but the point is that if there
are several chunks on the diff, failing one of them then it doesn't makes
sense to apply the other, the patch only makes sense to be applied at once
as a single item.

El 21/8/2016 20:38, "Kevin Decker" notifications@github.com escribió:

I disagree. The user should decide how they want to handle this. They may
want to have everything possible apply and be notified about what did not.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#131 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAgfvswxopCWa1llTBih1CNLk9jdw5EDks5qiJsIgaJpZM4JpS2J
.

[kpdecker]

That's exactly what fuzzy diffing is, is it not? Regardless, I don't want
the library to make this decision for the user when they can do it
themselves.

On Sun, Aug 21, 2016 at 1:41 PM Jesús Leganés Combarro <
notifications@github.com> wrote:

I was thinking about this posibility too, but the point is that if there
are several chunks on the diff, failing one of them then it doesn't makes
sense to apply the other, the patch only makes sense to be applied at once
as a single item.

El 21/8/2016 20:38, "Kevin Decker" notifications@github.com escribió:

I disagree. The user should decide how they want to handle this. They may
want to have everything possible apply and be notified about what did
not.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#131 (comment),
or mute the thread
<
https://github.com/notifications/unsubscribe-auth/AAgfvswxopCWa1llTBih1CNLk9jdw5EDks5qiJsIgaJpZM4JpS2J

.

—
You are receiving this because you modified the open/close state.

Reply to this email directly, view it on GitHub
#131 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAL_JqjC7fLWuYfCg7mS39oE_1SEEHVuks5qiJvlgaJpZM4JpS2J
.

[piranna]

That's exactly what fuzzy diffing is, is it not?

No, fuzzy diffing is about one patch can't be directly applied on the position it's originally defined, what I'm talking about here are the chunks inside a single patch: if a chunk can't be applied, the full patch must be discarded because it can be applied.

Regardless, I don't want the library to make this decision for the user when they can do it themselves.

I disagree on that, but it's your library. Just curious, can you give a use case where an user would not want to discard a chunk of a patch that can't be applied and apply the patch partially?

[kpdecker]

Perhaps I'm not understanding what the input data is here. Since you're going to have to write a test to get coverage on this, how about showing me the test input that would cause this.

[kpdecker]

Sorry, I did not remember what this API did exactly. My argument is slightly different, but still had the same outcome. Since this is not atomic, if a particular file fails but the ones prior to it succeed, then the system will be left in an indeterminate state that the API is not smart enough to recover from, as some file may have been changed and others not. By simply throwing vs. returning a list of the files that have failed, the caller is also not able to recover from the error.

[piranna]

the system will be left in an indeterminate state that the API is not smart enough to recover from

It could be done... but I admit it would not be so simple.

By simply throwing vs. returning a list of the files that have failed, the caller is also not able to recover from the error

This list should be done by the user anyway, isn't it? The API is not giving it...