On Wed, 2019-02-13 at 15:07 +0100, Ján Tomko wrote:
> On Wed, Feb 13, 2019 at 02:18:32PM +0100, Andrea Bolognani wrote:
> > Are the backslashes at the end of lines necessary?
>
> In this patch? Yes. The aim is to preserve the test coverage done before
> and after.
>
> > I've tried
> > removing a bunch of them and the test didn't break. Are the files
> > even valid JSON with the backslashes included?
>
> No, they get stripped by virTestLoadFile
Oh, I see, they're there to keep the input JSON as a single line
as it was originally. Makes sense.
> > Additional question: can't we pretty-print at least the input
> > files now? Unless of course the point of these specific test cases
> > is to prove we can successfully parse certain unusual constructs.
>
> The whole point of separating these was to allow easier changes
> and make it more-readable, so introducing more whitespace by removing
> the backslashes and prettifying it is possible.
Alright, it can be done in separate commits then.
[...]
> All the error messages match the original test. Guess it would make more
> sense to alter them before copying them.
Yeah, that way they get improved for FromString tests as well.
> > I think it would also look more legible if this entire if block was
> > inside the else branch of the previous if block, but if you feel
> > strongly about this version then just leave it as is.
>
> Like this?
>
> if (!injson) {
> if (info->pass) {
> return 0;
> } else {
> return -1;
> }
> } else {
> if (!info->pass)
> return -1;
> }
That's exactly what I had in mind.
--
Andrea Bolognani / Red Hat / Virtualization
