From: David G. <go...@py...> - 2012-03-12 03:44:09
|
On Sun, Jan 22, 2012 at 15:45, Guenter Milde <mi...@us...> wrote: > On 2012-01-20, David Goodger wrote: >> On Fri, Jan 20, 2012 at 05:01, Guenter Milde <mi...@us...> wrote: >>> On 2010-12-28, Eli Bendersky wrote: > >>>> I'm using docutils programmatically for converting ReST to HTML >>>> (specifically docutils.core.publish_from_doctree). Since the usage is >>>> programmatic (controlled by my application) I must have full control of >>>> exceptions for logging. However, I noticed that the FileInput class >>>> used by docutils for various purposes exits the application (sys.exit) >>>> when it can't find a file. > > ... > >>> The easiest fix (with the above side-effect of a distracting >>> error message) would be to change the default behaviour: > ... >>> - autoclose=True, handle_io_errors=True, mode='rU'): >>> + autoclose=True, handle_io_errors=False, mode='rU'): > ... > >> -1 on this change. If we only do this, we'll be bombarded by error >> reports from users who mis-typed their command lines. This is a >> *feature*. > > I agree that this should be only part of the solution. > >> The issue seems to be that the handle_io_errors parameter is not >> accessible from client code. Turning it off doesn't make it >> accessible. Either we should make it accessible, or remove it >> completely and handle errors elsewhere. > >>> For a nice user experience this should be combined with a better response >>> to missing "main" input files (catching the error and reporting as >>> "severe" system message at an more appropriate place). > >> I think the current error handling *is* at the most appropriate place. >> If you disagree, please suggest a place to do the error handling. The >> problem is, removing error handling from there would require >> duplicating code in multiple places elsewhere. > > IMV, the right place is core.Publisher.report_Exceptions. > Please see my patch below for a possible implementation. > >> Instead, how about adding a command-line/config option to control >> this? Then programmatic users can handle any exceptions themselves, >> and regular users won't see any change. > > I don't think that an additional config setting is necessary, as the > "traceback" setting already switches between passing the exception to the > caller or catching and reporting exceptions by > core.Publisher.report_Exceptions. OK >> Problem: only docutils.io.FileInput & FileOutput have handle_io_errors >> parameters, none of the other docutils.io classes do (they don't need >> them, but would were we to generalize). We don't currently pass >> settings objects to the docutils.io classes, so we have two choices. >> Either add settings parameters to all appropriate docutils.io classes, >> or add handle_io_errors parameters to the StringInput & Output >> classes. I vote for the latter. Either way, we'll have to change >> calling code to pass in appropriate values depending on the settings >> value. > > With "handle_io_errors" for all docutils.io classes, we could pass :: > > handle_io_errors=not(settings.traceback) > > to the io.FileInput/Output classes in core.Publisher.set_source() and > core.Publisher.set_destination. Sure. > This also allows to change the default value to ``False``, as throughout > docutils FileInput is called 5x with handle_io_errors=False and specific > error handling but only one time with handle_io_errors=True. Hold on. Best not to flip the default like that. There could be 3rd-party code out there that uses it. > However, I prefer to (first deprecate and later) remove the > handle_io_errors argument from io.File(In|Out)put and raise specific > exceptions instead. This enables core.Publisher.report_Exceptions() > to clear these errors. If we deprecate handle_io_errors, let's not change its default first. Any code out there that depends on the default *will break*, and that's unacceptable. It would be better to rip out the error-handling code altogether (I'm not suggesting we do this though). I'm worried that reversing the default will cause third-party code to break without warning. Maybe there is no such code, but we have to assume that there is, and we can't just reverse the default without any indication that there has been a change. It's hard to articulate, but this approach just feels *wrong*. Idea: change the default to None. Check for True and False values, and issue a deprecation warning for *both* cases. Docutils itself uses that code, and will have to be fixed. Eventually, remove the handle_io_errors parameter altogether. > This way, error handling is done in one place and it is not > necessary to pass arguments around with every instantiation of > FileInput or FileOutput. Instead, using one of the io.File* classes > for file input/output will "automacigally" do the right thing for > both, programmatic and command line use. In general I agree. > The following patch does not (yet) remove the error-handling code in > docutils.io, nor the "handle_io_errors" argument from the calls > elsewhere. The 10 lines of additional code are more than offset by 15 > lines of obsoleted code. Let's add a deprecation warning before removing anything. To make such a change without warning would be very rude and feels wrong. How about starting a branch for this? It's complicated enough that it would be useful for collaboration & thorough testing. -- David Goodger <http://python.net/~goodger> |