About the quality of this repository

[m-vdb]

Hi dear maintainers,

I've been pretty active on this repository for a long time now, and I must say that I'm pretty pissed with the quality of the code. I know that THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND but, still, you cannot just put your official python SDK out there and say "oh it's fine if its broken, just use PHP instead".

Where I'm working, we've been using the Facebook Ads API for several years and went through all the different migration adventures, not without breaking a sweat. I must say that when I saw the changelog for the 2.6 version, I told myself "Piece of cake". Yeah, right. I've opened 4 issues on this repo so far:

• AbstractObject.__init__ initializes _data with None key. #222
• use self.get_parent_id_assured() if api_create() method exists #223
• 2.6.2 version published on pypi is wrong #224
• Typechecker support for custom classes #221

And the deadline to migrate on this version is set 5 days from now, July 13th.

But that's not all. You introduced an auto-generation of code. I don't know who had this idea but it's pretty far from genius. So I get it, you don't want to have to maintain 250+ python classes manually. But hey, have you thought about inheritance? Have you thought about meta classes? Instead here is what we get:

• obfuscated code: I didn't know it could exist in Python, but you made it possible
• almost impossible collaboration: a nice guy wants to improve your data model? he can't
• poorly designed python code: yes, python is supposed to be a scripting language. But do you think the Django guys are writing scripts?
• non-standard code: have you tried to run pep8 or pylint on your codebase?

Oh and that's still not all. There is simply no unit tests. The only ones that are here were coded by collaborators not working at Facebook. Today, you have:

• 32 unit tests
• 7 integration tests (BTW, do you even run them?)
• 250+ python files

Don't you think that instead of auto-generating python code you could have written unit tests?

Last but not least, today you have 30 opened issues/PRs, and some have been opened for a long time. Can you improve your processes regarding that matter? I think it's sad that people are spending time on this SDK and you just ignore their work.

Of course I'm gonna continue to use this repo because I have no other choice, but I and other developers would be very glad if you could work on those improvements.

Have a good day,
Max.

[rituparnamukherjee]

@m-vdb,

cc @JiamingFB
Thank you for your feedback on this. We did think that this migration to the autogenerated framework will not be easy. The reason behind doing this was to have more consistency between the SDK and the API since many features were not supported in the SDK before. We will try to incorporate the changes that we are seeing in terms of pull request and issues, and will also try to be more proactive on responding to these moving forward.

We are also working on generating unit test to have as much coverage as possible. This is work in progress and please bear with us till we can release something more concrete. Having said that we do have breadth of tests which we run on live api to see how the SDKs are doing, and for 2.6 we made sure that all those tests passed. But yes it is hard to have tested all cases and that is why we did expect incoming issues on the release of this. We will stabilize the autogenerated version and hopefully the next migration will be much smoother. And yes we also have encountered issues with linters and that is because of the autogenerated code, it is hard to incorporate the linter dependencies for different languages in the autogen framework for this and that is why the issue.

We apologies for the inconvenience and will try to be more proactive on this.

[m-vdb]

Thanks for your answer. I understand the challenges you face at Facebook and the need to autogenerate a framework, but I still think it's a mistake. If you stay with this autogeneration thing, do you have plans to ease collaboration? Is there any way to improve the diffs of the commits? More atomicity? etc...

I appreciate the transparency in your comment but it's a pity that we do not have access to such builds or at least their statuses. To circumvent that we even had to run the integration tests ourselves, which is rather hard to maintained.

Finally, let me just say that I'm a bit surprised to have found these issues. We're at 2.6.2, 2 minors after the initial 2.6.0 release and no one complained either internally or externally. The only things I tried to do weren't exotic at all: campaign creation, adset creation, ad creation, creative creation and updates. Nothing out of the ordinary. Nothing funky with videos and the like. And still your tests failed to discover these issues? It wouldn't take much time to at least unit test your base classes...

[rituparnamukherjee]

I think we missed them because of some missing tests around batch mode. We will try to be more vigilent.

[x8lucas8x]

Totally agree with the @m-vdb. In the last 2.11 release (see diff), for instance, you've removed the create_product_catalog method from the Business class. But not only that, you've also removed the api_create method from the ProductCatalog class, which means that in theory we are only left with remote_create() for creating product catalogs. But wait, according to https://developers.facebook.com/docs/marketing-api/reference/product-catalog the new product catalog endpoint is /{business_id}/owned_product_catalogs. Guess what, the same 2.11 release is referring to the old endpoint (i.e. /{business_id}/product_catalogs).

But as if that was not enough, you've no consistent versioning. Please use semantic versioning properly for god's sake. It's 2017. It should be clear for developers when they should be wary of possible api breaking changes like that. On the same semantic versioning subject, I don't understand why you start the patch number with 1 as if you've fixed a bug for each major increment. That's totally non standard, but seems to be recurrent as one can see on https://github.com/facebook/facebook-python-ads-sdk/tags.

[codytwinton]

It seems like this issue has been open for more than a few months now. We have made many changes in the SDK since then, including releasing a new version. If you can repro your specific issue with the latest version of the SDK, please create a new issue or comment here with further details.