[libvirt] [PATCH] check-symfile: Use pythonesque string formatting instead of perl
Martin Kletzander
mkletzan at redhat.com
Thu Nov 28 10:43:01 UTC 2019
On Tue, Nov 26, 2019 at 10:29:14AM +0000, Daniel P. Berrangé wrote:
>On Tue, Nov 26, 2019 at 11:25:28AM +0100, Pavel Hrdina wrote:
>> On Tue, Nov 26, 2019 at 10:01:11AM +0000, Daniel P. Berrangé wrote:
>> > On Tue, Nov 26, 2019 at 09:48:31AM +0100, Michal Privoznik wrote:
>> > > On 11/26/19 9:24 AM, Erik Skultety wrote:
>> > > > On Mon, Nov 25, 2019 at 05:17:54PM +0100, Michal Privoznik wrote:
>> > > > > On 11/25/19 4:58 PM, Erik Skultety wrote:
>> > > > > > On Mon, Nov 25, 2019 at 04:37:36PM +0100, Peter Krempa wrote:
>> > > > > > > Commit d30a1ad0443 translated the symbol file checker from perl to
>> > > > > > > python by doing a literal translation in most cases. Unfortunately one
>> > > > > > > string formatting operation was not really translated into python
>> > > > > > > leaving users with non-helpful error:
>> > > > > > >
>> > > > > > > 'Symbol $1 is listed twice'
>> > > > > > >
>> > > > > > > Signed-off-by: Peter Krempa <pkrempa at redhat.com>
>> > > > > > > ---
>> > > > > > > scripts/check-symfile.py | 2 +-
>> > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
>> > > > > > >
>> > > > > > > diff --git a/scripts/check-symfile.py b/scripts/check-symfile.py
>> > > > > > > index 0c02591991..34396b8623 100755
>> > > > > > > --- a/scripts/check-symfile.py
>> > > > > > > +++ b/scripts/check-symfile.py
>> > > > > > > @@ -52,7 +52,7 @@ with open(symfile, "r") as fh:
>> > > > > > > line = line.strip(";")
>> > > > > > >
>> > > > > > > if line in wantsyms:
>> > > > > > > - print("Symbol $1 is listed twice", file=sys.stderr)
>> > > > > > > + print("Symbol %s is listed twice" % line ,file=sys.stderr)
>> > > > > >
>> > > > > > Not a deal breaker, but IMO should at least the "new" syntax for string
>> > > > > > formatting using the .format() method (works both with python 2 and 3).
>> > > > > >
>> > > > > > Ideally, we'd move to python 3.6+ (since 2 will die in about 2 months) and
>> > > > > > started using string interpolation (or f-strings if you want).
>> > > > >
>> > > > > Well, looks like we are not using that anywhere. And frankly, f-strings are
>> > > > > horrible. This is the most readable style for us, C developers IMO.
>> > > >
>> > > > Can you be more specific on what exactly is horrible about f-strings? IMO it's
>> > > > actually very intuitive way of formatting strings unlike using the '%'
>> > > > formatting sign where depending on whether you have 1 or multiple arguments you
>> > > > may or may not need to use a tuple. F-strings are also a bit faster than the
>> > > > other formatting methods and because they're evaluated during runtime, you can
>> > > > evaluate arbitrary expressions, even call functions.
>> > >
>> > > That's exactly what I find horrible. Just consider the following example:
>> > >
>> > > print(f'a={f(x,n):d}, b={g(x,n):d}')
>> > >
>> > > IMO the following is more readable:
>> > >
>> > > print("a=%d, b=%d" % (f(x,n), g(x,n)))
>> > >
>> > > Once again, I'm talking about C developers (me specifically). I don't doubt
>> > > that an experienced python developer finds f-strings a step forward.
>> >
>> > I've plenty of python dev experiance and I still think f-strings are
>> > a horrible regression in design. IMHO it is a good thing to have the
>> > parameters visually separated from the string, as it makes it clearer
>> > to read the string being formatted.
>>
>> I tend to agree here. f-strings are not that bad but it allows you to
>> write ugly code. It is probably good for interactive python but
>> otherwise I would prefer the new-style .format() with named arguments:
>>
>> "Hello {name}".format(name="Pavel")
>>
>> The major benefit that I see in this style is that it helps translators
>> to figure out what the substitution will be and the order of arguments
>> doesn't matter and you can build the dictionary passed to format()
>> outside of it's call.
>
>Note those advantages are already possible with traditional syntax
>
> "Hello %(name)s" % { name: "Pavel"}
>
>but for our build scripts this is academic as we're never going to
>do translation of anything in them.
>
The advantage of "".format() is that you cannot make one mistake that can happen
with the currently used version and that is that sometimes you might need to
make sure the variable is in a tuple, i.e. difference between `params` and
`(params,)`. Even though this is *very* corner case, you still don't know if
the word after `%` is the only parameter or the iterable of parameters to be
used. Even though this is not *that* corner case, the mistakes that would cause
this would probably be caught quickly and auxiliary build scripts are not
something that needs to care about translations.
Since the biggest bikeshedding^Wconversations usually revolve around the
smallest patches, I felt like expressing myself as well =)
But anyway, even though "%d" is more C-like, when talking about the parameters, the `.format()` is more C-like IMHO. More importantly it should be the same in all of the code, so that it is consistent. And *most* importantly!!! It is just a auxiliary build script, just make sure it works and that's it =D
Have a nice day (with my preferred colour of the bikeshed),
Martin
P.S.: Just to illustrate the first mentioned difference, try reading the
following code. I think the `.format` makes it more understandable:
def a(*args):
print("{}".format(args))
def b(*args):
print("{}".format(*args))
def c(*args):
print("%r" % (args,))
def d(*args):
print("%r" % (*args,))
def e(*args):
print("%r" % args)
a('asdf')
b('asdf')
c('asdf')
d('asdf')
e('asdf')
a('asdf', 'fdsa')
b('asdf', 'fdsa')
c('asdf', 'fdsa')
#d('asdf', 'fdsa') # it easy to see why this fails, but the code is gross
#e('asdf', 'fdsa') # it is not easy to see why this fails
a()
#b() # it is easy to see why this fails
c()
#d() # it easy to see why this fails, but the code is gross
#e() # it is not easy to see why this fails
>Regards,
>Daniel
>--
>|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
>|: https://libvirt.org -o- https://fstop138.berrange.com :|
>|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
>
>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20191128/d9210fa9/attachment-0001.sig>
More information about the libvir-list
mailing list