Lucas_Werkmeister_WMDE added a comment.
I guess I should start with what I had in mind in some form – I imagined the implementation would look something like this, repeated a lot of times: diff --git a/src/Formatters/LatLongFormatter.php b/src/Formatters/LatLongFormatter.php index a10970aef8..765f672218 100644 --- a/src/Formatters/LatLongFormatter.php +++ b/src/Formatters/LatLongFormatter.php @@ -194,9 +194,14 @@ private function getSpacing( string $spacingLevel ): string { } private function formatLatitude( float $latitude, float $precision ): string { + $northSymbol = $this->options->getOption( self::OPT_NORTH_SYMBOL ); + if ( !is_string( $northSymbol ) ) { + throw new InvalidArgumentException( self::OPT_NORTH_SYMBOL . ' must be a string' ); + } + return $this->makeDirectionalIfNeeded( $this->formatCoordinate( $latitude, $precision ), - $this->options->getOption( self::OPT_NORTH_SYMBOL ), + $northSymbol, $this->options->getOption( self::OPT_SOUTH_SYMBOL ) ); } Relative to this, an `$options->getStringOption()` method could be seen as a way to reduce code duplication – whereas it’s not clear to me how `setStringOption()` fits in. That doesn’t necessarily mean I’m vetoing it, I just don’t understand your proposal yet. > Are you for typed getters? Or are they just "nice to have"? Nice to have, I guess, but there’s probably not really a reason //not// to have them? Unless we change the approach in some way I’m not seeing at the moment. > Do you insist that the solution should throw an exception? Or swallow exceptions? Or are you open to both, or neither? I guess that’s actually a product question, in the end…? When users make API requests with invalid options, should we return an error or ignore the invalid options? (I think I would prefer throwing an exception which the API turns into an error, but I’d be fine with both.) > In terms of checking existing code, that seems quite a broad search scope. Do you have any concrete examples in mind of best practices that you would like to follow / worst practices you would like to avoid? What evidence could I supply to convince you that there is no such code? That’s a fair question ^^ I sidestepped it by just taking a look myself now, with `grep -rFA5 --color=yes '>getOption(' | less -R` in `vendor/data-values/`. I found three places that seemed to do any kind of checking of options: - DmsCoordinateParser, OPT_DEGREE_SYMBOL <https://github.com/DataValues/Geo/blob/1dd742dabb63e211862486259b2cbe0274211bf9/src/Parsers/DmsCoordinateParser.php#L154-L163> – **throws an exception** if the degree symbol isn’t found in the “coordinate segment”, whatever exactly this means. (Note that this is technically a parser, with `ParserOptions` instead of `FormatterOptions`. But they’re similar enough in principle that I think it’s still useful here.) - LatLongFormatter, OPT_PRECISION <https://github.com/DataValues/Geo/blob/1dd742dabb63e211862486259b2cbe0274211bf9/src/Formatters/LatLongFormatter.php#L143-L155> – **ignores the option** if it’s not a string, float or int. - LatLongFormatter, OPT_FORMAT <https://github.com/DataValues/Geo/blob/1dd742dabb63e211862486259b2cbe0274211bf9/src/Formatters/LatLongFormatter.php#L236-L260> – **throws an exception** if the format isn’t one of four well-known strings (though it also depends on the precision, I think?). Tons of other `getOption()` calls had no obvious error handling or type checking of any kind. So at the moment my impression is there is barely any existing code, and what little code exists isn’t even consistent, so we’re free to come up with whatever we want. > Is it mandatory for you that any proposed solution includes your `withDefaultOption` change? Definitely not. If you can find a good use for it, maybe it’s worth including, otherwise it can stay shelved until some future GB&C as far as I’m concerned. TASK DETAIL https://phabricator.wikimedia.org/T323778 EMAIL PREFERENCES https://phabricator.wikimedia.org/settings/panel/emailpreferences/ To: ArthurTaylor, 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