Re: [PATCH 001/103] virJSONValueObjectAddVArgs: Add 'k' convertor for formatting non-negative integers

On a Friday in 2021, Kristina Hanicova wrote: > On Thu, Oct 7, 2021 at 6:43 PM Ján Tomko <jtomko(a)redhat.com&gt; wrote: > > > On a Thursday in 2021, Peter Krempa wrote: > > >In many cases we use a signed value, but use the sign to note that it > > >was not assigned. For converting to JSON objects it will be handy to > > >have possibility to do this automatically. > > > > > >Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; > > >--- > > > src/util/virjson.c | 7 ++++++- > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > >diff --git a/src/util/virjson.c b/src/util/virjson.c > > >index 9adcea4fff..70ea71b505 100644 > > >--- a/src/util/virjson.c > > >+++ b/src/util/virjson.c > > >@@ -115,6 +115,7 @@ virJSONValueGetType(const virJSONValue *value) > > > * > > > * i: signed integer value > > > * j: signed integer value, error if negative > > >+ * k: signed integer value, omitted if negative > > > * z: signed integer value, omitted if zero > > > * y: signed integer value, omitted if zero, error if negative > > > * > > >@@ -189,6 +190,7 @@ virJSONValueObjectAddVArgs(virJSONValue *obj, > > > > > > case 'z': > > > case 'y': > > >+ case 'k': > > > case 'j': > > > case 'i': { > > > int val = va_arg(args, int); > > >@@ -200,7 +202,10 @@ virJSONValueObjectAddVArgs(virJSONValue *obj, > > > return -1; > > > } > > > > > >- if (!val && (type == 'z' || type == 'y')) > > >+ if (val == 0 && (type == 'z' || type == 'y')) > > >+ continue; > > >+ > > > > Please split out this cosmetic style change. > > > > Please don't. > Why not? It is unrelated to the goal of introducing the 'k' parser and having it here distracts from that goal. Writing a new commit might cost a few seconds of the author's time, but it will save time for the readers of such commit. And you can expect a commit to be read more times (even by the author a few years into the future, after they long forgot about writing it), while it is only written once. There might be some projects (I think linux kernel does this) that do not accept commits with purely whitespace/style changes, but we do allow them in libvirt and I'm much happier to review them, since they make the actual functional changes more obvious. (And they make the review of a 100-patch series possible). I like this blog post danpb wrote on the topic: https://www.berrange.com/posts/2012/06/27/thoughts-on-improving-openstack...