Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.Sign up
From mcculls on October 10, 2009 11:28:36
Guice lets you provide your own type converters using:
Original issue: http://code.google.com/p/google-guice/issues/detail?id=436
From mcculls on October 10, 2009 08:35:31
(whoops, accidentally hit return!)
OK, so Guice lets you provide your own type converters using:
but there's no way to get access to Guice's own type converters for primitive types.
Summary: Provide access to Guice's own internal TypeConverters
From limpbizkit on October 10, 2009 09:37:27
If you'd like to submit a patch that adds these constants to the interface, that would
Note that Guice's type conversion facility isn't really general-purpose; I suspect our
From mcculls on October 11, 2009 21:08:22
OK, will try out a few options and attach the best patch. The approach I'm using has
From mcculls on October 12, 2009 03:34:08
I took a slightly different approach, and added a "convertConstant" method to the
I've attached a proposed patch of the various changes, but need to add unit tests.
From limpbizkit on October 12, 2009 16:06:33
I gotta reject this patch because it overreaches the Guice injector to become a general
I'd like to avoid supporting weak converters when I know that it's insufficient for
That said, there is a market for a general purpose type converter framework. I'm not
From mcculls on October 12, 2009 22:04:31
> I gotta reject this patch because it overreaches the Guice injector to become
That's disappointing, without this patch I'll have to duplicate functionality already
BTW, here's how I came up with this patch...
Your original suggestion was to simply expose the ~10 internal type converter
I considered adding an API/SPI to list/query the installed set of type converters
Therefore I decided to provide a type-safe "convertConstant" method, which linked the
> We actually have a full type converter framework that's built upon Guice, but
Hehe, knowing there's a great proprietary system out there isn't much comfort ;)
> I'd like to avoid supporting weak converters when I know that it's insufficient
Sure, but this is really just allowing clients to apply the same conversions that
> That said, there is a market for a general purpose type converter framework.
Unfortunately I'm on a tight schedule. We were hoping to replace Plexus completely
Oh well, back to the drawing board...
From mcculls on October 15, 2009 08:11:46
FYI, decided to use a locally patched build for now as this feature also lets us use
From mcculls on October 15, 2009 09:04:14
Sure, that would be acceptable. I thought providing a "convertConstant" method would
From mcculls on October 15, 2009 11:04:17
Hmm... looks like adding a "getTypeConverterBindings()" method will be a much larger
Any suggestions? My gut still suggests the "convertConstant" approach is safer...
From limpbizkit on October 15, 2009 18:48:01
I'd like to expose the TypeConverterBinding, even if we need to make its constructor
Another benefit of this approach is that we could get ConvertedConstantBinding to
From mcculls on October 16, 2009 01:53:22
New patch that adds getTypeConverterBindings() to the injector and exposes the type
Couple of points about the patch:
From mcculls on May 04, 2010 01:23:25
This patch (GUICE_ISSUE_436_20100504.patch) exposes a new method in the Injector:
which returns the list of type converters registered with the injector, including
which lets you find out which type converter was used to convert that constant.
This patch makes TypeConverterBinding public and uses this as a replacement for the
From mcculls on November 18, 2010 03:09:46
Where does it expose the internal API? The intent of this patch is to extend the SPI to allow clients to introspect and discover what converters are registered with the injector and find out which converter converted a particular value (both valuable features if you want to know what's going on wrt. value conversion).
From sberlin on November 18, 2010 05:03:39
Mainly in that it exposes a TypeConverterBinding instance, but looking up in the comments it appears Jesse's OK with that. I'll try to grab his ear to talk to him.
Also, a question: The patch changes Errors.Converter.appliesTo to add a null check for 'o', which seems to be used in the Errors.convert method. When would someone want to convert 'null'? Why would we want to ignore it (as a potential source of mistake/failure)? I didn't see any obvious places looking in the caller where null was acceptable.
From mcculls on November 18, 2010 06:30:52
You'll see the NPE issue whenever you report a custom error message that includes a null parameter.
String value = null;
Won't actually add the error message to the final output, but instead you'll see an NPE stack trace below it:
Caused by: java.lang.NullPointerException
which then hides the original message.
It should be possible to use null values in error messages hence the null check.
[ this is also related to Issue 432 : https://code.google.com/p/google-guice/issues/detail?id=432 ]
From limpbizkit on November 18, 2010 08:28:48
I'm okay with the proposed change but like Sam I'm slightly anxious about the change in behaviour around errors.
I also think Set is the right type rather than List, because order is not significant and because each type converter can exist only once.
From mcculls on November 18, 2010 08:37:08
Wrt. the Errors change - what if I (as a user) want to add a binder error message that contains a potentially null message parameter? ie. like binder.addError("Some error, value=%s", value); where value might possibly be null.
This is not an unusual situation, but at the moment this will cause an NPE inside the format conversion code in Guice, wiping out the original error message and replacing it with a misleading stack trace that suggests the problem lies inside Guice rather than the true cause.
This is also inconsistent with the javadoc for Binder.addError() which states that String.format() will be used to format the message, and String.format() can handle null message parameters whereas currently Errors cannot.