fabriciocolombo / delphi-rest-client-api

A Delphi REST client API to consume REST services written in any programming language.
Apache License 2.0
380 stars 182 forks source link

More changes. #24

Closed thomaserlang closed 10 years ago

thomaserlang commented 10 years ago

Fixed the TList size bug in the superobject. Added event OnError/OnConnectionLost that can retry an request. Added Patch method An empty response will no longer create a new empty object. An HTTP 404 error will no longer raise an exception, returns nil instead. Added ISO 8601 to the superobject. Enable with {$DEFINE ISO8601}.

RobertoSchneiders commented 9 years ago

Why ignore the 404 error?

thomaserlang commented 9 years ago

I like the object to be nil instead of raising an exception on 404 errors.

One example that i find simpler:

user := TUser.get(1);
if not assigned(user) then
  raise ECustomException.create('The user was not found');

But it's properly bad default behavior.

I believe it would be possible to ignore the exception for 404 errors by using the OnError event and setting the retry mode to ignore instead of the default: raise.

Then an exception could be raised by default for 404 errors.

RobertoSchneiders commented 9 years ago

In my case, the return is string and can not be nil, so I have to check the errorcode, which is bad.

ronaldhoek commented 9 years ago

Why not make it optional and keep the original situation as default behaviour?

RobertoSchneiders commented 9 years ago

I don't think this should be the default behavior. As I said in the pull request, handle a single HTTP error code differently from the rest makes no sense to me.

We can create some optional configuration, but in my view, the default behavior should be raise an exception. Not because is the best practice, because the library has working in this way since his creation.

We can add an option to not raise exception on any HTTP errors. What do you think? (It's better discuss this on the PR page, right?)

Can you explain your idea of the optional implementation?

ronaldhoek commented 9 years ago

My idea is the same as yours. Default behaviour is to raise an exceptions (I thought this was the default current behaviour). Optionally allow developer to specify not to raise HTTP-errors and return the appropriate HTTP-error code (which the developer needs to check off course)

The way you allow the developer to not raise an exception on HTTP error's might be done using an event like thomaserlang said.

Ex. THTTPErrorEvent = procedure(Sender: TObject; HTTPCode: Integer; var RaiseException: Boolean);

Default 'RaiseException' will be true

thomaserlang commented 9 years ago

The event is already there: OnError (https://github.com/fabriciocolombo/delphi-rest-client-api/blob/0345e23246c9b4f4dc0a4307f9f2f64ba1ce8bf6/src/HttpConnection.pas#L28).

Change the value of ARetryMode from hrmRaise to hrmIgnore and it should work.

So yes, i agree that it should raise the 404 error per default.

thomaserlang commented 9 years ago

I have made a pull request the raises the 404 by default. f89ad24fe770b70779e6afd0437a5776721f9ae7