Automattic / wp-cldr

Use CLDR localization data in WordPress
GNU General Public License v2.0
24 stars 6 forks source link

refactor out get_cldr_item() #77

Closed stuwest closed 8 years ago

stuwest commented 8 years ago

@jblz, back in the day, when the plugin handled a single data structure like used in territory and language names, we added an intermediate method -- get_cldr_item() -- to look up an item in a locale bucket.

As we've expanded the data items available, there are now different data structures involved. get_cldr_item() feels like an extra layer, a bit of complexity that usually trips me up when I've been away from the code for a while.

This PR refactors it out. It's pretty straightforward -- we only use get_cldr_item() for territory names and language names. In both cases, we already have a public function that generates an array of all values.

One thing that was in get_cldr_item() and is now missing -- and has always been missing from the other public methods -- is argument validation. The docblocks call for strings, but we aren't confirming anywhere that the passed arguments are in fact strings. We could add that throughout the class something like:

        foreach ( func_get_args() as $argument ) {
            if ( ! is_string( $argument ) ) {
                return '';
            }
        }

It seems like a lot of code clutter just to avoid issues for developers who don't follow the documentation so I'm leaning against it. What do you think?

jblz commented 8 years ago

Looks good & tests well. This really simplifies those functions, imo. Good job.