Skip to content

Conversation

pobtastic
Copy link

All I've done here is to install the latest version of https://github.com/swagger-api/swagger-codegen and run;

./run-in-docker.sh generate -i https://api.sendinblue.com/v3/swagger_definition.yml -l php -o /gen/out/sendinblue -DpackageName=SendinBlue

This is for issue #186.

@deguif
Copy link

deguif commented Dec 12, 2022

This is already done on @preetishishodia7 PR: #242

@pobtastic
Copy link
Author

That branch has maybe not been generated from swagger, it’s not useable. Seems to also contain debug https://github.com/sendinblue/APIv3-php-library/pull/242/files#diff-3c23d7885d1a6e19ddc2271d9fb3dec8ba86bebc8d90454102620cb706266ee3R757 (note the failing tests too).

@deguif
Copy link

deguif commented Dec 13, 2022

It should be better now. At least CI is green ;)

@pobtastic
Copy link
Author

pobtastic commented Dec 13, 2022

All those files say not to update them manually - that they're auto-generated using Swagger. Swagger has literally just had the last compatibility PR merged - hence, this PR is simply the result of an auto-generation using the latest Swagger.

Now I'm not 100% completely sure, but seeing as my PR and #242 differ SO much - that it seems to me like all the changes made there have been done manually?

https://github.com/sendinblue/APIv3-php-library/blob/master/lib/ApiException.php#L26
* Do not edit the class manually.

@deguif
Copy link

deguif commented Dec 13, 2022

@preetishishodia7 is one of the top 3 contributors of this library so I would trust her for updating this library the right way ;)

@pobtastic
Copy link
Author

Ha, I don't want to be misunderstood - I'm sure whatever has been done is great. But really, that PR should also remove all lines relating to that the code has been autogenerated and not to change the files? Because from whenever that PR is merged onwards, that information is no longer correct.

I only need this PR up to use the https://github.com/sendinblue/APIv3-php-library/pull/244.patch file for our own composer file. I didn't use the other PR initially because of the debug and failing tests, but yeah - I'm happy to close this PR when the #242 PR is merged/ updated.

@pobtastic
Copy link
Author

You know ... now I look at it, lines from the autogenerated files have been changed for months.

Perhaps a separate PR should be created to update the README in the root, and remove all the "autogen" lines in the library files?

@deguif
Copy link

deguif commented Dec 13, 2022

I think the lines are still autogenerated (maybe via a custom mustache template), so technically the autogen lines are valid ;)

@pobtastic
Copy link
Author

Hmmmm it would be better if they were added to this repo really then? Just, the instructions suggest this isn't the case- and that the entire library can be autogenerated by just anyone using that Swagger project 😕

Meh, I'm happy to close this PR then and pivot over to #242 - I'll post in the issue and ask about the moustache templates 👍

Merci beaucoup

@pobtastic pobtastic closed this Dec 13, 2022
@pobtastic pobtastic deleted the issue-186-php8-compatibility branch December 13, 2022 15:48
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