Re: [PATCH 03/22] qapi/source: Remove line number from QAPISourceInfo initializer

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> With the QAPISourceInfo(None, None, None) construct gone, there's not
> really any reason to have to specify that a file starts on the first
> line.
>
> Remove it from the initializer and have it default to 1.
>
> Remove the last vestiges where we check for 'line' being unset. That
> won't happen again, now!
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/parser.py |  2 +-
>  scripts/qapi/source.py | 12 +++---------
>  2 files changed, 4 insertions(+), 10 deletions(-)
>
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index b378fa33807..edd0af33ae0 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -47,7 +47,7 @@ def __init__(self, fname, previously_included=None, 
> incl_info=None):
>          if self.src == '' or self.src[-1] != '\n':
>              self.src += '\n'
>          self.cursor = 0
> -        self.info = QAPISourceInfo(fname, 1, incl_info)
> +        self.info = QAPISourceInfo(fname, incl_info)
>          self.line_pos = 0
>          self.exprs = []
>          self.docs = []
> diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
> index 21090b9fe78..afa21518974 100644
> --- a/scripts/qapi/source.py
> +++ b/scripts/qapi/source.py
> @@ -37,10 +37,9 @@ def __init__(self) -> None:
>  class QAPISourceInfo:
>      T = TypeVar('T', bound='QAPISourceInfo')
>  
> -    def __init__(self, fname: str, line: int,
> -                 parent: Optional['QAPISourceInfo']):
> +    def __init__(self, fname: str, parent: Optional['QAPISourceInfo'] = 
> None):

Not mentioned in the commit message: you add a default parameter value.
It's not used; there's just one caller, and it passes a value.
Intentional?

>          self.fname = fname
> -        self.line = line
> +        self.line = 1
>          self._column: Optional[int] = None
>          self.parent = parent
>          self.pragma: QAPISchemaPragma = (
> @@ -59,12 +58,7 @@ def next_line(self: T) -> T:
>          return info
>  
>      def loc(self) -> str:
> -        # column cannot be provided meaningfully when line is absent.
> -        assert self.line or self._column is None
> -
> -        ret = self.fname
> -        if self.line is not None:
> -            ret += ':%d' % self.line
> +        ret = f"{self.fname}:{self.line}"
>          if self._column is not None:
>              ret += ':%d' % self._column
>          return ret

Mixing f-string and % interpolation.  I doubt we'd write it this way
from scratch.  I recommend to either stick to % for now (leave
conversion to f-strings for later), or conver the column formatting,
too, even though it's not related to the patch's purpose.