I have the following notes to the event handling (alternatively custom
error handling):
+1 for enum for severities
Makes sense to align with JsonLocation and get rid of URI.
There is no list of events yet. I am not sure if we need it.
The reason behing custom error handling is to allow to turn off some
fail-fast errors.
The Optional is stressing that given method is not required to return value.
I prefer to return boolean from handle method and not to introduce
handled() method into Event.
I am not that perfectly happy getting rid of event handling mechanism. I
think this is still needed to support even simple usecases where default is
too restrictive.
MartinV
On Wed, Apr 29, 2015 at 11:37 PM, Eugen Cepoi <cepoi.eugen_at_gmail.com> wrote:
> Ahh I didn't read correctly your comment :) I thought you where saying
> "add a handled method to the event handler" not the event... my fault.
> The event can be immutable, lets keep it this way. Returning true/false
> looks like a common pattern to indicate success/failure or
> hanlded/unhandled. But if we were to drop this error handling system I'd be
> fine too...not sure if people do really need it.
>
> 2015-04-29 23:03 GMT+02:00 Romain MB <rmannibucau_at_tomitribe.com>:
>
>> 2015-04-29 22:13 GMT+02:00 Eugen Cepoi <cepoi.eugen_at_gmail.com>:
>> >
>> >
>> > 2015-04-29 21:56 GMT+02:00 Romain MB <rmannibucau_at_tomitribe.com>:
>> >>
>> >> Hi Martin,
>> >>
>> >> here are my feedbacks on the code:
>> >>
>> >> 1. severities should be an enum (I think it will not be open)
>> >> 2. JsonbEventLocator more or less exists in JSON-P,any way to merge
>> >> them? (ie have a json-common api -
>> >>
>> https://docs.oracle.com/javaee/7/api/javax/json/stream/JsonLocation.html
>> ).
>> >
>> >
>> > I would prefer to not see a json-common :)
>> > If jsonb was built on top of jsonp we wouldn't have this dilemma.
>> >
>> >>
>> >> Plus I wouldn't use Optional here, it will mainly be used for logging
>> >> and optional API is just too verbose for the most common cases IMO.
>> >> Finally does the URI really means anything? We eat streams/readers so
>> >> I wouldnt see URI as so important (in particularly since on write side
>> >> it doesnt mean anything most of the time for the mapper framework).
>> >
>> >
>> > It could be an optional<XXLocationXXX> and the location just contain the
>> > basic position infos. I don't suggest here to define another "Location"
>> for
>> > jsonb.
>> >
>> >>
>> >> 3. on the handler signature: I'd return void and add a handled()
>> >> method to the event to make the API more explicit
>> >
>> >
>> > No I think it's better to return true/false, otherwise the impl would be
>> > stateful and not thread safe.
>> >
>>
>> the event is already stateful anyway. it doesnt change the handler
>> chain so not sure i get your point
>>
>> >>
>> >>
>> >> On the overall solution I have 2 points:
>> >> 1. is it really needed for 1.0? Can't we just use direct exceptions
>> >> like in JSON-P? I'm very tempted to wait for users feedbacks and see
>> >> if it is missing before adding it.
>> >>
>> >> 2. Do we already have somewhere a list of events? They should be in an
>> >> enum + in TCK to ensure portability otherwise kept open it is not
>> >> something portable so not something the spec should handle directly
>> >> IMO (as a user I would then expect to unwrap my mapper to a vendor
>> >> type to access it, not to use directly the spec).
>> >>
>> >>
>> >>
>> >>
>> >> Romain Manni-Bucau
>> >> @rmannibucau
>> >> http://www.tomitribe.com
>> >> http://rmannibucau.wordpress.com
>> >> https://github.com/rmannibucau
>> >>
>> >>
>> >> 2015-04-29 19:53 GMT+02:00 Martin Vojtek <voytoo_at_gmail.com>:
>> >> > Hi Experts,
>> >> >
>> >> > I have pushed changes which include proposal to customize error
>> >> > handling.
>> >> >
>> >> > Summary of the proposal:
>> >> >
>> >> > Erros, warning and other issues should be (similarly to JAXB)
>> propagated
>> >> > as
>> >> > events. It will be possible to intercept (and to ignore) these
>> events by
>> >> > providing custom JsonbEventHandler implementation.
>> >> >
>> >> > Details could be found in specification and in code (api + examples).
>> >> >
>> >> > MartinV
>> >
>> >
>>
>
>