Automattic / wp-cldr

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

Add custom exception & magic methods for overloading #78

Closed stuwest closed 5 years ago

stuwest commented 8 years ago

@akirk added some commits in #76 to create a custom exception type and add a magic __set() and __get() methods for overloading. To focus discussion, I moved these commits to their own branch add/exception-class and this PR. I added another commit to with some docblock (HT phpcs with the full WordPress coding standard) and rebase cleanup.

@akirk, couple questions:

  1. What about using is_wp_error() instead of try/catch/throw exception? I'm hazy on why we'd use one or the other.
  2. for the custom exceptions, do we need the numeric $message_ids? seems it could be more useful if the exceptions threw a specific message (e.g. "The WordPress locale ary doesn't have a corresponding CLDR locale loaded; see ___ to download it.") than a generic exception constants.
  3. The overloading methods seem complicated for what we need which is more like what is in the current set_locale(). Do you see other ways we'd use them?

@jblz, any thoughts?

jblz commented 8 years ago

What is the problem we're trying to solve here? It's probably just my jetlag-riddled brain, but I'm having trouble discerning what magic getter / setters on the locale affords us here.

I also don't really understand the rationale behind a custom exception class. Seems like returning a WP_Error here & checking for it with is_wp_error would work ok & be less code.

akirk commented 8 years ago

The main benefit of the magic getters and setters is that we can set the locale by a simple assignment while assuring that the appropriate JSONs are loaded.

Possibly returning a WP_Error would also work, I'll do some further experimentation. See it as a step on the way to get decent error handling :)

stuwest commented 5 years ago

going to close this one for now as it's been a few years.