Added Format.Builtin.toString() as they tend to show up in exception.#87
Conversation
| * @return Short name for built-in formats as they tend to show up in exception messages. | ||
| */ | ||
| @Override | ||
| public String toString() { |
There was a problem hiding this comment.
I think it would be cleaner to define a private constructor that accepts a String name parameter and toString simply returns the name. It would also fix the leak in this class - its called "Buildin", but people can create more instances of it, which wasn't the intention.
There was a problem hiding this comment.
Agree. I wasn't sure whether the public constructor was intentional.
In this case, I would propose changing Builtin to an enum, which automatically also fixes the toString() issue!
There was a problem hiding this comment.
My bad, enum is not possible due to the Format generic.
Made the constructor private instead.
|
LGTMToo. Thanks @sjoerdtalsma and @yurishkuro . |
| */ | ||
| @Override | ||
| public String toString() { | ||
| return "Builtin." + name; |
There was a problem hiding this comment.
btw. you can do: return Builtin.class.getSimpleName() + "." + name;
There was a problem hiding this comment.
@sjoerdtalsma do you want to make this change or should we merge this PR? thanks.
There was a problem hiding this comment.
@bensigelman Thanks for the mention; I'll modify the PR; this change is trivial, looks like I missed @pavolloffay 's comment..
|
Last activity on this PR is over a week ago and there have been releases of |
|
@sjoerdtalsma merged, seem good enough. Thanks!! |
5 participants
For example, using Brave with Jersey, the following exception message showed up: