Re: [PATCH v2 17/25] binman: Add a consistent way to report errors with fit

2022-03-05 Thread Simon Glass
Hi Alper,

On Thu, 3 Mar 2022 at 14:16, Alper Nebi Yasak  wrote:
>
> On 24/02/2022 02:00, Simon Glass wrote:
> > Add a new function to handling reporting errors within a particular
> > subnode of the FIT description. This can be used to make the format of
> > these errors consistent.
> >
> > Signed-off-by: Simon Glass 
> > ---
> >
> > (no changes since v1)
> >
> >  tools/binman/etype/fit.py | 33 +
> >  tools/binman/ftest.py |  2 +-
> >  2 files changed, 26 insertions(+), 9 deletions(-)
>
> Reviewed-by: Alper Nebi Yasak 
>
> I don't like entry.Raise(msg) in general, if it were me I would define
> Exception subclasses and e.g. raise CustomError(self, subnode) for the
> various error conditions, but that's beside the point.

I have so far stuck with ValueError as I find the need to import
classes to raise an exception a pain. But I suppose that would be OK.

>
> > diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
> > index 70966028e8..50a9179f9f 100644
> > --- a/tools/binman/etype/fit.py
> > +++ b/tools/binman/etype/fit.py
> > [...]
> > @@ -273,6 +274,20 @@ class Entry_fit(Entry_section):
> >
> >  return tools.read_file(output_fname)
> >
> > +def _raise(self, base_node, node, msg):
> > +"""Raise an error with a paticular FIT subnode
> > +
> > +Args:
> > +base_node (Node): Base Node of the FIT (with 'description' 
> > property)
> > +node (Node): FIT subnode containing the error
> > +msg (str): Message to report
> > +
> > +Raises:
> > +ValueError, as requested
> > +"""
> > +rel_path = node.path[len(base_node.path) + 1:]
>
> Can base_node be anything except self._node here, why not use that?

OK I fixed that.

>
> > +self.Raise(f"subnode '{rel_path}': {msg}")
>
> The names are a bit inconsistent, but I guess you intend to change
> Entry.Raise() to Entry._raise() later on...

Well I was planning to change Raise() to raise(), so I'll rename this
raise_subnode()

>
> > +
> >  def _build_input(self):
> >  """Finish the FIT by adding the 'data' properties to it
> >
> > [...]

Regards,
Simon


Re: [PATCH v2 17/25] binman: Add a consistent way to report errors with fit

2022-03-03 Thread Alper Nebi Yasak
On 24/02/2022 02:00, Simon Glass wrote:
> Add a new function to handling reporting errors within a particular
> subnode of the FIT description. This can be used to make the format of
> these errors consistent.
> 
> Signed-off-by: Simon Glass 
> ---
> 
> (no changes since v1)
> 
>  tools/binman/etype/fit.py | 33 +
>  tools/binman/ftest.py |  2 +-
>  2 files changed, 26 insertions(+), 9 deletions(-)

Reviewed-by: Alper Nebi Yasak 

I don't like entry.Raise(msg) in general, if it were me I would define
Exception subclasses and e.g. raise CustomError(self, subnode) for the
various error conditions, but that's beside the point.

> diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
> index 70966028e8..50a9179f9f 100644
> --- a/tools/binman/etype/fit.py
> +++ b/tools/binman/etype/fit.py
> [...]
> @@ -273,6 +274,20 @@ class Entry_fit(Entry_section):
>  
>  return tools.read_file(output_fname)
>  
> +def _raise(self, base_node, node, msg):
> +"""Raise an error with a paticular FIT subnode
> +
> +Args:
> +base_node (Node): Base Node of the FIT (with 'description' 
> property)
> +node (Node): FIT subnode containing the error
> +msg (str): Message to report
> +
> +Raises:
> +ValueError, as requested
> +"""
> +rel_path = node.path[len(base_node.path) + 1:]

Can base_node be anything except self._node here, why not use that?

> +self.Raise(f"subnode '{rel_path}': {msg}")

The names are a bit inconsistent, but I guess you intend to change
Entry.Raise() to Entry._raise() later on...

> +
>  def _build_input(self):
>  """Finish the FIT by adding the 'data' properties to it
>  
> [...]