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

XE5+ changes #39

Open reckface opened 9 years ago

reckface commented 9 years ago
  1. Rename TRestClient to TJsonRestClient to avoid collisions with native TRestClient
  2. Introduce Post functions to support returning of data sets
  3. Introduce Display name parameter to make created datasets with user friendly titles
  4. Fall back to stNull to ftString instead of ftUnknown
  5. Expand the use of superobject since native Delphi serialization/Json handling is not as robust
RobertoSchneiders commented 9 years ago

@reckface I merged some PRs and this PR is now conflicting. Can you fix this?

The name change will be a big break change, but, I don't think is a big problem to us. What do you think @fabioxgn?

@thomaserlang can you review this PR?

thomaserlang commented 9 years ago

I'm fine with the TRestClient name change.

Why isn't there a "PutJSON" and "PatchJSON"? Isn't possible to call them "Post, put and patch" and just overload them?

I agree that the other JSON libs sucks. The official Delphi JSON serialization does not work for half of my valid JSON data. I would prefer Superobject as the default. Even though the code is a big mess, it's possible to change. I have long thought about building a much simpler version.

RobertoSchneiders commented 9 years ago

For me SuperObject can be the default too, but, the USE_SUPER_OBJECT directive is used in a few more places, I think.

Rather than comment the directive in RestClient.pas, It wouldn't be better to change this

{$IFNDEF DELPHI_XE_UP}
  {$DEFINE USE_SUPER_OBJECT}
{$ENDIF}

for this:

  {$DEFINE USE_SUPER_OBJECT}

in DelphiRest.inc, don't you think?

thomaserlang commented 9 years ago

I agree. It should still be possible to use another library.

fabioxgn commented 9 years ago

@RobertoSchneiders The only problem with name change is that it will break existing PRs, but just leave this one to be merged last and we are ok.

But wouldn't it be a better option to use a namespace instead of renaming the class? We already had problems with DataSetUtils which we had one unit with the same name.

Or do we need to support Delphi versions that doesn't support namespaces?

If we do not have to support older versions, I'd prefer to namespace all the units instead of changing the class name, like:

JSONRestClient.TRestClient

reckface commented 9 years ago

Happy to change it back.

On Sat, Apr 11, 2015 at 2:57 PM, Fábio Gomes notifications@github.com wrote:

@RobertoSchneiders https://github.com/RobertoSchneiders The only problem with name change is that it will break existing PRs, but just leave this one to be merged last and we are ok.

But wouldn't it be a better option to use a namespace instead of renaming the class? We already had problemas with DataSetUtils which we had one unit with the same name.

Or do we need to support Delphi versions that doesn't support namespaces?

If we do not have to support older versions, I'd prefer to namespace all the units instead of changing the class name, like:

JSONRestClient.TRestClient

— Reply to this email directly or view it on GitHub https://github.com/fabriciocolombo/delphi-rest-client-api/pull/39#issuecomment-91847375 .

reckface commented 9 years ago

Just a thought. We can use namespaces when coding, but the package won't install to the Delphi IDE with that naming conflict.

On Mon, Apr 13, 2015 at 9:00 AM, Eti Kagbala reck@lineone.net wrote:

Happy to change it back.

On Sat, Apr 11, 2015 at 2:57 PM, Fábio Gomes notifications@github.com wrote:

@RobertoSchneiders https://github.com/RobertoSchneiders The only problem with name change is that it will break existing PRs, but just leave this one to be merged last and we are ok.

But wouldn't it be a better option to use a namespace instead of renaming the class? We already had problemas with DataSetUtils which we had one unit with the same name.

Or do we need to support Delphi versions that doesn't support namespaces?

If we do not have to support older versions, I'd prefer to namespace all the units instead of changing the class name, like:

JSONRestClient.TRestClient

— Reply to this email directly or view it on GitHub https://github.com/fabriciocolombo/delphi-rest-client-api/pull/39#issuecomment-91847375 .

ronaldhoek commented 9 years ago

You cloud add a deprecated alias... for backwards compatibility

type
  TRestClient = TJsonRestClient deprecated 'Use TJsonRestClient';
fabioxgn commented 9 years ago

@reckface but does anyone use the packages? There isn't a package for XE7 for example and anyone complained about it, I don't like the whole package idea and nobody is maintaining them. For me we could drop the packages all together.

reckface commented 9 years ago

I don't know enough about Delphi and packages, I just know I had difficulty with units not installed to my IDE (due to my lack of understanding). I have added the: TRestClient = TJsonRestClient deprecated 'Use TJsonRestClient'; as suggested and it works just fine. It appears I didn't need to install the package, but it's installed now. I have an entire set of units that take json (of a specific type) and builds a TClientDataset descendant called TAPIQuery. It acts like TADOQuery in that it has a SQL property, supports parameters, but the Open procedure calls a WebApi. This project of yours, has now made it possible to replace TADOQuery completely in my project. Would that be something that can be hosted on GitHub as a branch/fork of this project?

On Mon, Apr 13, 2015 at 1:32 PM, Fábio Gomes notifications@github.com wrote:

@reckface https://github.com/reckface but does anyone use the packages? There isn't a package for XE7 for example and anyone complained about it, I don't like the whole package idea and nobody is maintaining them. For me we could drop the packages all together.

— Reply to this email directly or view it on GitHub https://github.com/fabriciocolombo/delphi-rest-client-api/pull/39#issuecomment-92336929 .

RobertoSchneiders commented 9 years ago

About the packages, I think we can remove all of them. I don't think they are useful for anything at all.

@reckface It will probably be another library witch depends on delphi-rest-client-api. Try to put it in a GitHub repository and we will take a look.