Re: Improvements to Web IDL exception message strings
Hi, helpful exception messages are very important, and ensuring that sufficient context is present is a good contribution to that. Thanks for pushing this forward! When starting work on replacing uses of the deprecated ErrorResult::Throw(nsresult), I noticed that the existing error messages show a syntactical inconsistency. Some end with a period (.), some do not. To ensure a consistent presentation, we should define the preferred style for this, and apply it consistently. I think the options with their rationale are: 1. individual messages passed to Throw* should not end in a period, but should form a complete sentence otherwise and a period suffix might be added depending on how/where it is exposed 2. all individual messages passed to Throw* should end in a period, and they form a complete sentence; no additional period is ever added regardless of how they are exposed 3. individual messages passed to Throw* may or may not end in a period, depending on whether they form a complete sentence; no additional period is ever added regardless of how they are exposed I think either of the first two options should be chosen, with a slight preference for the first option. Are there other opinions on this? Maybe there are more options I didn't consider. Simon On Mon, Feb 10, 2020 at 3:23 PM Boris Zbarsky wrote: > > Hello all, > > We just made some changes to how exception strings from ErrorResult get > formatted [1]. The changes apply to both the Throw* DOMException > methods and ThrowTypeError/ThrowRangeError. > > In the new setup, exception messages have "%s: " prepended to them, > where %s is replaced with one of: > > * "InterfaceName.methodName", when the exception is from a method cal. > * "InterfaceName.attrName getter", when the exception is from a getter > * "InterfaceName.attrName setter", when the exception is from a setter > * "InterfaceName constructor", when the exception is from a constructor. > > The idea is to give web developers a bit more context about what callee > failed. > > I have tried to go through existing message strings and remove > duplication of that information where it existed. > > The changes only affect exceptions thrown via the ErrorResult that > bindings pass to implementation methods. In particular, Promise > rejections via Promise methods (as opposed to throwing on the passed-in > ErrorResult) do not benefit from these changes; more work will need to > happen there. > > The changes also do not affect exceptions thrown via > ErrorResult::Throw(nsresult). As a reminder, this is deprecated and > people should switch to the methods that provide useful error messages. > > Please let me know if there are concerns around this change, suggestions > for further improvements, etc! > > -Boris > > [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1613013 > ___ > dev-platform mailing list > dev-platform@lists.mozilla.org > https://lists.mozilla.org/listinfo/dev-platform ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Improvements to Web IDL exception message strings
On 2/10/20 9:57 AM, Anne van Kesteren wrote: Would using Document.prototype.getElementById be too long? That is a good question. I'm going to do a bit of refactoring so we only need to change one place to try these ideas out and would love some feedback! In the meantime, someone who wants to try this locally should apply the patches in https://bugzilla.mozilla.org/show_bug.cgi?id=1614471 and experiment a bit. -Boris ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Improvements to Web IDL exception message strings
On Mon, Feb 10, 2020 at 3:23 PM Boris Zbarsky wrote: > You can test this now in the console using: > >CSS.escape() /* no args */ >document.getElementById() /* no args */ > > My changes did not affect the error messages from those, note; just used > that same string in more places. Would using Document.prototype.getElementById be too long? I've sometimes seen developers advocate using # to mean .prototype (e.g., Document#getElementById via https://mathiasbynens.be/notes/javascript-prototype-notation), but I don't know how widespread that is. It does seem a little funky to use Document.getElementById. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Improvements to Web IDL exception message strings
Hello all, We just made some changes to how exception strings from ErrorResult get formatted [1]. The changes apply to both the Throw* DOMException methods and ThrowTypeError/ThrowRangeError. In the new setup, exception messages have "%s: " prepended to them, where %s is replaced with one of: * "InterfaceName.methodName", when the exception is from a method cal. * "InterfaceName.attrName getter", when the exception is from a getter * "InterfaceName.attrName setter", when the exception is from a setter * "InterfaceName constructor", when the exception is from a constructor. The idea is to give web developers a bit more context about what callee failed. I have tried to go through existing message strings and remove duplication of that information where it existed. The changes only affect exceptions thrown via the ErrorResult that bindings pass to implementation methods. In particular, Promise rejections via Promise methods (as opposed to throwing on the passed-in ErrorResult) do not benefit from these changes; more work will need to happen there. The changes also do not affect exceptions thrown via ErrorResult::Throw(nsresult). As a reminder, this is deprecated and people should switch to the methods that provide useful error messages. Please let me know if there are concerns around this change, suggestions for further improvements, etc! -Boris [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1613013 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Improvements to Web IDL exception message strings
On 2/8/20 2:31 PM, Domenic Denicola wrote: Would you consider using something that avoids the confusion with static methods/properties? E.g. - InterfaceName's methodName() - InterfaceName's attrName getter - InterfaceName's attrName setter We could think about doing that, yes. If we do, we should change all the existing messages that talk about InterfaceName.methodName (e.g. the ones that get thrown for invalid arguments, etc). The main goal here is to give web developers an idea of which method they were calling quickly, without taking up space from the actual error message which tells them what it was they did wrong in calling it. So there's a tension between brevity and getting the details right... Your suggestions are pretty decent on the brevity front, though, so that seems workable. I wonder what web developers think of the options here... I'm also curious what the output would be for actually-static methods, e.g. CSS.escape(), DOMPointReadOnly.fromPoint(), etc. It'd be "CSS.escape: ", etc. You can test this now in the console using: CSS.escape() /* no args */ document.getElementById() /* no args */ My changes did not affect the error messages from those, note; just used that same string in more places. -Boris ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Improvements to Web IDL exception message strings
On Friday, February 7, 2020 at 8:55:45 AM UTC-5, Boris Zbarsky wrote: > Hello all, > > We just made some changes to how exception strings from ErrorResult get > formatted [1]. The changes apply to both the Throw* DOMException > methods and ThrowTypeError/ThrowRangeError. > > In the new setup, exception messages have "%s: " prepended to them, > where %s is replaced with one of: > > * "InterfaceName.methodName", when the exception is from a method cal. > * "InterfaceName.attrName getter", when the exception is from a getter > * "InterfaceName.attrName setter", when the exception is from a setter > * "InterfaceName constructor", when the exception is from a constructor. Would you consider using something that avoids the confusion with static methods/properties? E.g. - InterfaceName's methodName() - InterfaceName's attrName getter - InterfaceName's attrName setter As-is it seems like it would be confusing to talk about, for example, Node.appendChild, which is === undefined. I'm also curious what the output would be for actually-static methods, e.g. CSS.escape(), DOMPointReadOnly.fromPoint(), etc. > > The idea is to give web developers a bit more context about what callee > failed. > > I have tried to go through existing message strings and remove > duplication of that information where it existed. > > The changes only affect exceptions thrown via the ErrorResult that > bindings pass to implementation methods. In particular, Promise > rejections via Promise methods (as opposed to throwing on the passed-in > ErrorResult) do not benefit from these changes; more work will need to > happen there. > > The changes also do not affect exceptions thrown via > ErrorResult::Throw(nsresult). As a reminder, this is deprecated and > people should switch to the methods that provide useful error messages. > > Please let me know if there are concerns around this change, suggestions > for further improvements, etc! > > -Boris > > [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1613013 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
