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 -- [email protected]
To unsubscribe send an email to [email protected]
