Re: [PATCH 4/4] pylibfdt: add FdtRo.getprop_or_none()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]



Hi Luca,

On Mon, 24 Jan 2022 at 13:50, Luca Weiss <luca@xxxxxxxxx> wrote:
>
> Hi Simon,
>
> On Montag, 24. Jänner 2022 18:57:45 CET Simon Glass wrote:
> > Hi Luca,
> >
> > On Sat, 22 Jan 2022 at 03:36, Luca Weiss <luca@xxxxxxxxx> wrote:
> > > Hi Simon,
> > >
> > > On Dienstag, 28. Dezember 2021 09:34:57 CET Simon Glass wrote:
> > > > Hi Luca,
> > > >
> > > > On Sat, 25 Dec 2021 at 06:26, Luca Weiss <luca@xxxxxxxxx> wrote:
> > > > > Add a new method that doesn't throw an exception when a property isn't
> > > > > found but returns None instead.
> > > > >
> > > > > Also add a test for the new method.
> > > >
> > > > You can use
> > > >
> > > > getprop(node, prop, quiet=QUIET_NOTFOUND)
> > >
> > > This returns -1 when not found which I found quite un-pythonic (not that
> > > pylibfdt is currently super pythonic ;) ).
> > > Having None returned if not found is much nicer to use and much more clear
> > > to use
> > >
> > >     if foobar is None:
> > >  instead of having to use
> > >
> > >     if foobar == -QUIET_NOTFOUND:
> > > Are there any changes I can do for you to reconsider your position on this
> > > patch?
> >
> > I think I was put off by the mention of an exception in the commit
> > message. Really all you are doing is trying to make this function more
> > pythonic. I suppose we could change the getprop() function to return
> > None if there is no property (with QUIET_NOTFOUND supplied for the
> > 'quiet' argument). That makes not-found special, but I suppose if the
> > error were anything else, e.g. -FDT_ERR_BADSTRUCTURE and
> > quiet=[BADSTRUCTURE] were passed, we could return None in that case
> > too?
> >
> > I agree it is more pythonic.
>
> The "proper" way (in my opinion) would be to return the property or None,
> except for more serious errors like the BADSTRUCTURE one that just throws an
> exception; and hide all the C integer return values inside the wrapper.
> And instead of passing quiet=[BADSTRUCTURE] you would do

You could return None for the ones that are quietened, but then note
that you cannot recover the actual error. So in fact I think that is a
problem with what I proposed above, so in fact we should only return
None for -FDT_ERR_NOTFOUND, if even that.

>
> try:
>     myprop = fdt.getprop(foobar)
> except FdtBadStructureError:
>     foobar()
>
> I understand it's probably not a good idea to change the existing function too
> much given current users where scripts depend on this behavior but this way
> it's definitely more approachable to an average Python user.

Unless you are possibly expecting an error, e.g. -FDT_ERR_NOSPACE. It
is a pain to have to catch exceptions for what is normal operation. In
fact if I recall the Python sages recommend against things like that?

Regards,
Simon


> >
> > David, what do you think?
> >
> > Failing that I'm OK with this function, but with the commit message
> > updated to not mention exceptions, as we already have that mechanism,
> > so it is confusing.
> >
> > BTW you can use QUIET_NOTFOUND instead of [FDT_ERR_NOTFOUND]
> >
> > Regards,
> > Simon
>
>
>
>




[Index of Archives]     [Device Tree]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux