From: David G. <go...@py...> - 2009-10-29 14:18:27
|
On Thu, Oct 29, 2009 at 05:18, Guenter Milde <mi...@us...> wrote: > On 2009-10-28, David Goodger wrote: >> On Wed, Oct 28, 2009 at 10:08, <mi...@us...> wrote: > > ... >>> Modified: trunk/docutils/docutils/parsers/rst/__init__.py >>> =================================================================== >>> --- trunk/docutils/docutils/parsers/rst/__init__.py 2009-10-27 07:56:50 UTC (rev 6187) >>> +++ trunk/docutils/docutils/parsers/rst/__init__.py 2009-10-28 14:08:17 UTC (rev 6188) >>> @@ -169,15 +169,14 @@ >>> instead! >>> """ > >>> - def __init__(self, level, message, source, line): >>> + def __init__(self, level, message, spot): > >> This is changing the API! > > Is it? > > * The change is done to the init method of the "internal" DirectiveError > exception:: > > class DirectiveError(Exception): > > """ > Store a message and a system message level. > > To be thrown from inside directive code. > > Do not instantiate directly -- use `Directive.directive_error()` > instead! > """ > while the "public" Directive.directive_error() arguments remain unchanged! > > * The "source" and "line" arguments were added *by me* just some weeks > ago (in the first attepmt to fix the error reporting for included > files) and never made public. I stand corrected. So *that* was the original API change. :-) How do you define "public" and "private" here? How do you know that the earlier change wouldn't break some code (like Sphinx, or Epidoc, etc.), that intimately use Docutils as a library? Any API change has the potential of breaking client code. One way to avoid such breakage is to use default parameter values (``source=None, line=None``). Experimenting on the code is what branches are for. The trunk should be stable and not have API changes without discussion first. >> Is this really necessary? > > Not imperatively. But cleanung up the code early on seems better to me > than keeping cruft until someone starts to use it... What cruft are you referring to? >> It seems frivolous. I don't recall any discussion of this. > > It was done after careful consideration and remembering the "commit if > you are sure but be prepared to revert" rule set up by you. > >> Are you absolutely sure that no client code anywhere will break because >> of this? I'm not! > > Yes, I am. How? >> Please justify the change or revert it, immediately. > > Do you still want me to revert? > > If yes, which state: > > a) completely broken error reporting (pre 0.6), > > b) previous fix with "source" and "line" added to DirectiveError > without asking, > > c) "forward revert" implementing the last commits feature with two arguments > instead of one? As a matter of convention, I'd prefer the two explicit arguments instead of one obfuscated one. Why use a dictionary here? The comment "(partially) Fix reporting for problems in included files" didn't fill me with confidence. The use of a dictionary replacing two explicit arguments just seemed wrong. I didn't realize that this was a reworking of an earlier API change. Overall, I'm not confident that this improves the situation. I'd prefer to see the work being done in a branch. There you'd be free to experiment without anyone (me) second-guessing you. -- David Goodger <http://python.net/~goodger> |
From: Guenter M. <mi...@us...> - 2009-10-29 22:00:55
|
On 2009-10-29, David Goodger wrote: > On Thu, Oct 29, 2009 at 05:18, Guenter Milde <mi...@us...> wrote: >> On 2009-10-28, David Goodger wrote: >>> On Wed, Oct 28, 2009 at 10:08, <mi...@us...> wrote: >> ... >>>> Modified: trunk/docutils/docutils/parsers/rst/__init__.py >>> This is changing the API! >> * The change is done to the init method of the "internal" DirectiveError >> exception ... >> while the "public" Directive.directive_error() arguments remain unchanged! > How do you define "public" and "private" here? How do you know that > the earlier change wouldn't break some code (like Sphinx, or Epidoc, > etc.), that intimately use Docutils as a library? Because I rely on them using Docutils as advised:: >> class DirectiveError(Exception): ... >> Do not instantiate directly -- use `Directive.directive_error()` >> instead! With this clear instruction in place for years, I do not think special care must be taken to prevent breaking code that ignores it. > Any API change has the potential of breaking client code. One way to > avoid such breakage is to use default parameter values (``source=None, > line=None``). > Experimenting on the code is what branches are for. The trunk should > be stable and not have API changes without discussion first. I see and will stay away from API changes. Please decide, whether arguments of a function that should not be used from outside are still considered to be part of the API. > As a matter of convention, I'd prefer the two explicit arguments > instead of one obfuscated one. Agreed. In this case, I found that no argument is required at all. The DirectiveError is handled in states.run_directive(), which has access to the state machine and can request the info saved in the StringList via self.state_machine.get_source_spot(). > The comment "(partially) Fix reporting for problems in included files" > didn't fill me with confidence. Sorry for not beeing overly verbose and clear here. The commit resulted in a stable/functional state of the Docutils code with improved error reporting. The hithero commented out test case in test_include.py works as it should, all other tests stay intact without change. "Partially" refers to the fact that only *a part of* the calls to reporter.system_message() (or reporter.[info|error|warning|severe]) have been amended with arguments passing on the correct source/line information, so most system messages will still report that same (wrong) source and line number as in releases 0.5 and 0.6. > Overall, I'm not confident that this improves the situation. Just try test_include.py or any document that incudes a file with problems. > I'd prefer to see the work being done in a branch. There you'd be > free to experiment without anyone (me) second-guessing you. Günter |