Hey, On Thu, Nov 29, 2018 at 12:23:16PM -0500, Frediano Ziglio wrote: > > For example, when Marc-André added the json-glib dependency ( > > https://lists.freedesktop.org/archives/spice-devel/2018-August/045202.html > > ), we did not get first a patch adding the new dep and not doing > > anythingelse, and the rest of the series after this initial patch was > > merged. The addition of the new dep was sent at the same time as the > > patches which need json-glib. > > > > Usually following patches are also an explanation of the stuff you are > adding. In this case the usage, as you also said, is pretty clear.
It's clear it's going to be used to gather statistics, exactly how it would look, I have no idea. And I don't understand why there are no statistics gathering patches coming with it. Such patches don't even require the recorder to come first, as the recorder macros could probably temporarily use g_log or g_log_structured. So I don't really see a reason for not having one or two accompanying patches illustrating how it's going to be used. > A difference of this patch is that is not a simple library addition, > there's an option, a dummy replacement, a test and configure function. Yes, a separate patch for adding the recorder is good given it's a non trivial change (wrt the size of the patch). > What you probably want to say is "I'd prefer the merge to be done > with some code really making use of this library". Yes, that's it, though it's not just the merge, but even for the review. Easier to comment about a new internal API when you can see how it's used. > If that the case however should not be an excuse to avoid to review it > and discuss. Regarding reviewing the dependency addition, I don't think I had a lot to say about the patch itself (I can take a look again). Christophe
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
