| Message ID | 20210422030720.3685766-4-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qapi: static typing conversion, pt5a | expand |
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.
On 4/24/21 2:38 AM, Markus Armbruster wrote: > 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? > No. Leftover from an earlier version where it was used. It can be made to always be an explicit parameter now instead.
On 4/24/21 2:38 AM, Markus Armbruster wrote: > 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. True. Two thoughts: 1. I don't like using % formatting because it behaves differently from .format() and f-strings. My overwhelming desire is to never use it for this reason. Example: {foo} will call foo's __format__ method, whereas "%s" % foo will simply add str(foo). They are not always the same, not even for built-in Python objects. 2. Cleaning up the formatting here without cleaning it up everywhere is a great way to get the patch NACKed. You have in the past been fairly reluctant to "While we're here" cleanups, so I am trying to cut back on them. This is why my habit for f-strings keeps trickling in: whenever I have to rewrite any interpolation, I reach for the one that behaves most idiomatically for Python 3. I am trying to balance that against churn that's not in the stated goals of the patch. In this case: I'll clean the rest of the method to match; and add a note to the commit message that explains why. I will get around to removing all of the f-strings, but I want to hit the clean linter baseline first to help guide the testing for such a series. I regret the awkward transitional period. --js
John Snow <jsnow@redhat.com> writes: > On 4/24/21 2:38 AM, Markus Armbruster wrote: >> 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. > > True. Two thoughts: > > 1. I don't like using % formatting because it behaves differently from > .format() and f-strings. My overwhelming desire is to never use it for > this reason. > > Example: {foo} will call foo's __format__ method, whereas "%s" % foo > will simply add str(foo). They are not always the same, not even for > built-in Python objects. I only care for readability, which profits from local consistency. Maybe I'll sing a different tune once I got actually bitten by the difference between interpolation and f-strings. > 2. Cleaning up the formatting here without cleaning it up everywhere > is a great way to get the patch NACKed. You have in the past been > fairly reluctant to "While we're here" cleanups, so I am trying to cut > back on them. Yes, I've been pushing back on such cleanups. But it's really a case-by-case issue. When a patch fits on a page, squashing in a bit of losely related cleanup is usually fine. When it's long, keep it focused on a single purpose. > This is why my habit for f-strings keeps trickling in: whenever I have > to rewrite any interpolation, I reach for the one that behaves most > idiomatically for Python 3. I am trying to balance that against churn > that's not in the stated goals of the patch. > > In this case: I'll clean the rest of the method to match; and add a > note to the commit message that explains why. Okay. > I will get around to > removing all of the f-strings, The opposite, I presume. > but I want to hit the clean linter > baseline first to help guide the testing for such a series. I regret > the awkward transitional period. I'd leave converting interpolation to f-strings for later. I can tolerate early, partial conversion, since I trust complete conversion will happen, and as long as the resulting local inconsistency isn't too grating. Subjective, I know.
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): 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
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(-)