fromString returning `false` in case of error is not pythonic

[allan-simon]

Which break the principle of "least surprise"

i.e most python libraries raise exception instead (ask for forgiveness not permission) so one could choose to not handle them , or handle them at a higher level

also it breaks static analysis tool that will say that

fromstring(DIFF).apply(dir)

is not valid because false has no apply methods

[uilianries]

@allan-simon Hello! Could you please elaborate a bit more your case? Is it possible to provide a reproducible example? Please, take a look in the issue #19 as reference, that's good way to reproduce the case. Regards.

[allan-simon]

sorry what I mean is that

def fromstring(s):
  """ Parse text string and return PatchSet()
      object (or False if parsing fails)
  """
  ps = PatchSet( StringIO(s) )
  if ps.errors == 0:
    return ps
  return False

should be

def fromstring(s):
  """ Parse text string and return PatchSet()
      object (or False if parsing fails)
  """
  ps = PatchSet( StringIO(s) )
  if ps.errors == 0:
    return ps
  raise Exception()

as it's very uncommon in python to have a function that returns false for error case (it's more something PHP used to do )

(fromstring and other methods that return false as a way to convey an error occured )

[uilianries]

@allan-simon Thank you for your prompt response! Your explanation has clarified my perspective.

I agree that using Exceptions in these cases is more Pythonic and aligns better with best practices.

However, changing the current design would introduce breaking changes, which is something we need to avoid. This fork was specifically created to fix a bug that was impacting the Conan client (details here: https://github.com/conan-io/python-patch-ng?tab=readme-ov-file#why-did-we-fork-this-project). Altering this behavior could affect not only Conan but also other users who rely on the current design and expect False to be returned in their code.

Given this, I believe it's best to maintain the existing approach.

[xmo-odoo]

@uilianries maybe a solution would be to add an optional error check to PatchSet.__init__ that way people who want "hard" errors could call PatchSet(BytesIO(DIFF), check=True) and they'd get an exception if that was not a valid patch?

[uilianries]

@xmo-odoo Thank you for your suggestion, but it would introduce a new behavior that would need to be covered by tests and would require extra maintenance, so I would still keep the current behavior without changes. Regards!