Lucas_Werkmeister_WMDE added a comment.
In T323778#9441487 <https://phabricator.wikimedia.org/T323778#9441487>, @ArthurTaylor wrote: > My suggested approach would be first to make the libraries themselves robust and prevent them throwing type errors. To do this, I would deprecate the generic setOption and getOption functions, and replace them with {get,set}{String,Int,Bool}Option functions that are robust to bad input. Then we can catch InvalidFormatterOptions in our API code and do something more graceful there. Would be happy to take a look at this. I’m not sure what the benefit of typed setter functions is. The options can be set via an API parameter, and the API module doesn’t have any idea what the expected type is; it could use the type of the given value, but then I don’t think “call `setStringOption()` if it’s a string, `setBoolOption()` if it’s a bool, etc.” has any benefit over “call `setOption()` with whatever the value is”. Typed getter functions, on the other hand, could be useful to reduce duplicate code in the formatters. But I’m not yet sure what the getter should do if the option has been set to a different type than expected: return a default value (e.g. `null`) that the caller can handle (e.g. `$options->getString( self::OPT_NORTH_SYMBOL ) ?? 'N'`), or just throw an exception? I think we should check if there’s any existing code that already handles invalid options, and whether it does that consistently. I also notice that the formatters sometimes define default options upfront, like here in `LatLongFormatter`’s constructor: $this->defaultOption( self::OPT_NORTH_SYMBOL, 'N' ); $this->defaultOption( self::OPT_EAST_SYMBOL, 'E' ); $this->defaultOption( self::OPT_SOUTH_SYMBOL, 'S' ); $this->defaultOption( self::OPT_WEST_SYMBOL, 'W' ); Arguably we could infer the expected type here – if the default is a string, it’s reasonable to assume that any custom value must also be a string (perhaps with an escape hatch in case other types should be supported after all). I also have some open work (from a previous GB&C event) to replace `defaultOption()` (mutates instance) with `withDefaultOption()` (returns new copy), see pull request <https://github.com/DataValues/Interfaces/pull/59>, that I still want to get back to; perhaps we can combine this somehow. TASK DETAIL https://phabricator.wikimedia.org/T323778 EMAIL PREFERENCES https://phabricator.wikimedia.org/settings/panel/emailpreferences/ To: Lucas_Werkmeister_WMDE Cc: ArthurTaylor, ItamarWMDE, Michael, Lucas_Werkmeister_WMDE, Aklapper, Danny_Benjafield_WMDE, Astuthiodit_1, karapayneWMDE, Invadibot, maantietaja, Akuckartz, Nandana, Lahi, Gq86, GoranSMilovanovic, QZanden, KimKelting, LawExplorer, _jensen, rosalieper, Scott_WUaS, Wikidata-bugs, aude, Mbch331
_______________________________________________ Wikidata-bugs mailing list -- wikidata-bugs@lists.wikimedia.org To unsubscribe send an email to wikidata-bugs-le...@lists.wikimedia.org
