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

[Luca Weiss <luca-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org>]

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-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org> 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-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org> 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

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.

Regards
Luca

> 
> 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