Code review comment for lp:~salgado/bzr/bug-308122

Revision history for this message
Guilherme Salgado (salgado) wrote :

On Tue, 2009-12-22 at 14:51 +0000, John A Meinel wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Guilherme Salgado wrote:
> > So, to summarize, this is how it's going to look like, in the end:
> >
> > 1. cmd_unshelve will grow a 'preview' action
> >
> > 2. Unshelver.run() will accept an optional file descriptor, where a diff
> > will be written when the preview action is specified. When the fd is not
> > given it will fall back to ui_factory.make_output_stream().
>
> I'm not sure whether it should be on run() or as a constructor argument.
>
> unshelver = Unshelver.from_args(..., to_file=self.outf)
> or
> Unshelver.run(to_file=self.outf)

I think the former would be nicer, but only because we already have the
show_diff argument on __init__(), so I did that change.

>
> >
> > 3. Unshelver will grow a write_diff(merger, diff_output=None) method, to
> > be called when the 'preview' action is specified.
> >
>
> Seems reasonable.
>
> > 4. The preview action will cause the diff to be written to the fd, but
> > we don't want to output the changes (implicitly done by
> > merger.change_reporter, through tree_merger.make_preview_transform()),
> > so we need to silence it. Is it ok to silence it by stubbing out
> > merger.change_reporter.report inside Unshelver.write_diff()?
> >
> > How does that sound?
> >
>
> I don't like the idea of stubbing it out, as it seems brittle at best.
> If it is going to be difficult, then I would just say let it do the
> report and don't worry about it. (One *can* think of it as showing the
> diffstat before the diff...)
>
> I *think* that if we want to properly silence it, then we should just
> set the change reporter to the NullChangeReporter at the appropriate
> time. If that isn't obvious, then I would punt and just let the output
> get written to the terminal.

I could do that, but I'd prefer to just let the changes get written to
the terminal if you don't see that as a problem.

I've just pushed these changes, but I'm not quite happy with the
multiple assertions in the same test method to make sure the diff is
correct, so I'd appreciate suggestions to make it better.

--
Guilherme Salgado <email address hidden>

« Back to merge proposal