Code review comment for lp://staging/~elachuni/piston-mini-client/log-to-file

Revision history for this message
Anthony Lenton (elachuni) wrote :

Hi Michael,

> > +        api = AnnoyAPI()
> > +        with NamedTemporaryFile() as fp:
> > +            api.log_filename = fp.name
>
> Why not initialise the api the way you've recommended in the README?
> That is, with:

Heh, good point :) That test was written before the api accepted the log filename as a constructor and I forgot to update them later. I'll fix before landing.

> > +    @patch('httplib2.Http.request')
> > +    def test_perms_issue_doesnt_fail(self, mock_request):
> > +        mock_request.return_value = ({'status': '201'}, '"Go away!"')
> > +        api = AnnoyAPI()
> > +        forbidden_path = '/usr/bin/bash'
> > +        self.assertRaises(IOError, open, forbidden_path, 'a')
> > +        api.log_filename = forbidden_path
> > +
> > +        response = api.poke()
>
> Would it be worth having *some* output when the IOError is raised -
> perhaps to stdout? ("configured logfile is not writeable").

Trouble with that is that when we use piston-mini-client within a wsgi stack, writing to stdout could break things badly. I agree it should provide *some* feedback, possibly via logging, but it doesn't do any logging integration yet.

« Back to merge proposal