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

Feature.xe4 support2 #20

Merged
merged 3 commits into from
Nov 4, 2023
Merged

Conversation

yonojoy
Copy link
Contributor

@yonojoy yonojoy commented Oct 18, 2023

This is a replacement for pull request #17. All the JSON code is stuffed into one file.

@yonojoy
Copy link
Contributor Author

yonojoy commented Oct 18, 2023

I hope, it is right this way.
The code is only tested under XE4.

@yonojoy yonojoy mentioned this pull request Oct 19, 2023
@wlandgraf
Copy link
Contributor

Thank you, but still, OpenAPIJSON has too many differences. The idea is not to separate into classes and create abstract methods, but just add IFDEF where needed. Just keep the way the code was before, just add IFDEF where needed, without refactoring the classes.

Could you please do that?

Also, there are new modifications in OpenApiUtils.pas, it would be good to merge those changing into the pull request to avoid conflicts.

@yonojoy
Copy link
Contributor Author

yonojoy commented Nov 1, 2023

That's what I said in the first place: The two/three implementations do not share enough code to satisfy a common base class (other than an abstract base), therefore my first implementation with 3 distinct implementations and the trick to ifdef the right one in (to avoid the extra code for abstract base class). This way it is easy to see, what to do to support an other json lib. And if you compare OpenApiJson.pas with OpenApiJsonWrapper.pas you'll see, that the wrapper code for System.JSON is untouched. If you compare OpenApiJson.pas with OpenApiJsonWrapperFpc.pas you'll see, that the code for this is untouched, too. (In svn you would have seen this right away - git is shitty compared to svn in this area).

I understood your argument to have everything in one file (from a marketing perspective), therefore I followed your proposal and spent my time to stuff everything into OpenApiJson.pas.

You wrote:

Keeping separated classes is fine if you can have a single ancestor that share the common code, with virtual
methods, and for descendant classes only override and implement the differences. Is this possible?

I have done that.

But now you write:

Thank you, but still, OpenAPIJSON has too many differences. The idea is not to separate into classes and create
abstract methods, but just add IFDEF where needed.

Sorry, I am out at this point. I highly appreciate the work you did with openapi-delphi-generator and wanted to give something back. But reading your arguments, I think it is better to stay with your current implementation. Not that many developers will have to support such an old Delphi version. And if they have to, they can refer to my pull request. This way your wrapper class stays readable.

When programming, I always try to do the right thing: In this case your wrapper class is a bridge pattern. Normally you would have an abstract base class and two or three classes implementing the bridge. I have provided two variations of this idea. But -sorry- I am not willing to provide an implementation based on ifdefs.
If you ever decide to add support for another JSON lib, I think you'll remember this conversation.

What should we do?

@wlandgraf
Copy link
Contributor

If you don't mind, I will get to implement XE4 support then based on your pull request #20.

@yonojoy
Copy link
Contributor Author

yonojoy commented Nov 1, 2023

Yes, please feel free to use the code however you want.

@wlandgraf wlandgraf merged commit 237f642 into landgraf-dev:main Nov 4, 2023
@wlandgraf
Copy link
Contributor

@yonojoy, thank you very much for your contribution.
I was able to find time to review your changes, and now everything is applied (with the modifications you allowed me to do so).
Everything should be fixed and working, I'd appreciate if you could confirm everything is working one.

@yonojoy
Copy link
Contributor Author

yonojoy commented Nov 14, 2023

I tested your code. Everything seems to be fixed and working for XE4.

@yonojoy yonojoy deleted the Feature.XE4-Support2 branch November 14, 2023 11:54
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.

2 participants