Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XE5+ changes #39

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

XE5+ changes #39

wants to merge 4 commits into from

Conversation

reckface
Copy link

@reckface reckface commented Apr 7, 2015

  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

The Delphi XE5 already contains TRestClient, so this was renamed to TJsonRestClient
Additionally, TClientDataSet was added
Expanded the functionality to support HTTP Posts that returned datasets. Also  removed the directives that conditionally prevented the use of SuperObject. The Delphi generic based Json deserialization (unmarshalling) produced unpredictable results, whereas the superobject was more predictable.
The Field Definition can now have a display name instead of the json-determined fieldName
@RobertoSchneiders
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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

@fabioxgn
Copy link
Contributor

@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
Copy link
Author

Happy to change it back.

On Sat, Apr 11, 2015 at 2:57 PM, Fábio Gomes [email protected]
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
#39 (comment)
.

@reckface
Copy link
Author

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 [email protected] wrote:

Happy to change it back.

On Sat, Apr 11, 2015 at 2:57 PM, Fábio Gomes [email protected]
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
#39 (comment)
.

@ronaldhoek
Copy link
Contributor

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

type
  TRestClient = TJsonRestClient deprecated 'Use TJsonRestClient';

@fabioxgn
Copy link
Contributor

@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
Copy link
Author

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 [email protected]
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
#39 (comment)
.

@RobertoSchneiders
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants