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

Already on GitHub? Sign in to your account

Where do I start? #8

Open
jkbbwr opened this Issue Oct 1, 2013 · 7 comments

Comments

Projects
None yet
3 participants

jkbbwr commented Oct 1, 2013

Where do I start, there are countless issues with this API, let me address some of them here.

First is the need to patch an external module to fix issues with your API, you could bundling that package locally rather than relying on the user to install the package then modify it with your requirements.

There is no way to specify the location of the configuration file for the Client, it is just hard coded to a non absolute path.

########
##
##  Setup web service connectivity by getting need config data, security tokens etc.
##
########
class ET_Client(object):

Okay, style wise, this really needs to be a document string. The bracket of comments look ugly and take up 5 lines.

It would be far nicer to read

class Client(object):
    """Setup web service connectivity by getting needed config data, security tokens etc."""

The naming scheme in this project is inconstant at best. The naming of every class in this library is prefixed with ET_ this is a redundant naming convention and isn't used in Python. Sometimes variables and functions are defined as self.authToken and sometimes its self.refresh_token() and sometimes its def determineStack(self):
It really doesn't matter which one is used but stick to a style and use it.

ET_Client.py#L59

You are using an undefined variable before its defined. This will cause horrible errors during execution.

ET_Client.py#L864

Are those comments or another language? If so why are they presented as nop strings?

ET_Client.py#L936

## FIX THIS

Comments like this are just scary. At least throw me an undefined behaviour exception so I know something unplanned is happening.

ET_Client.py#L563

Most scary are your init methods.

def __init__(self):
    super

Has no effect at all.
If there is inherited behaviour these classes are relying on this could cause subtle breaks in the object.

When you have used super correctly you forgot to specify the where clause.

ET_Client.py#L504

You return values from __init__ in several places. This is going to cause runtime exceptions all over the place.

Other issues!

My lint and code smell tools alone detected 180 code issues, most of which were mostly naming and style violations but some of which are more obvious, let me list a few here

  • Raising a string.
  • Unbound local variables.
  • Catch All Exceptions.
  • Unused imports.

This is supposed to be a professional project. The code in this client library is horrid. I have spent a whole morning trying to modify it to bring it up to standard and I have given up after the mountain of major issues just kept piling up.

This code is totally unfit for production until someone really takes a chainsaw to it and cuts away everything that they can.

Contributor

creatovisguru commented Oct 1, 2013

It would be nice to see the work you've done already, we might be able to leverage it to help improve our code base for this Fuel SDK.

Could you please send a PR and we'll take a look at what you've done so we can improve the code quality?

We're working on improving the code base for our next release, which branch did you use for your testing? Our dot9 branch is our next planned release.

We will add a note to our README.md page stating that this product is in beta still.

jkbbwr commented Oct 1, 2013

Hey, thanks for getting back to what is a rather brutal attack on your codebase. I don't currently have any changes online and I threw most of them away this morning out of frustration. I don't mind helping to improve this project if it benefits the goals of my companies project in the long run. We really need to have a proper discussion about the API, and how it is used before I can really start repairing a copy of FuelSDK-Python,

Do you have skype or gmail?

Contributor

creatovisguru commented Oct 1, 2013

No worries. We have a really healthy community of developers with great documentation available at https://code.exacttarget.com. I'm guessing that's how you found this Fuel SDK, just in case you aren't part of our community. You should take a look around our documentation for APIs, code samples, and the general splendor that is our Code@ community for ExactTarget developers.

Are there there any specific questions about our APIs which you may have that could help encourage positive contributions? If you do, we'd welcome you to ask those and stir up discussion on our Code@ Q/A Page

jkbbwr commented Oct 1, 2013

I find that using Q/A boards never really get clear answers, if I could
have 30 minutes of your time tomorrow sometime, then I could figure out the
API fully and I could help you work on Fuel a bit too, but this will suit
us all better than asking a half baked question on Q/A and not getting a
complete and personalised answer.

On Tue, Oct 1, 2013 at 4:14 PM, Benjamin Dean notifications@github.comwrote:

No worries. We have a really healthy community of developers with great
documentation available at https://code.exacttarget.com. I'm guessing
that's how you found this Fuel SDK, just in case you aren't part of our
community. You should take a look around our documentation for APIshttps://code.exacttarget.com/api,
code samples https://code.exacttarget.com/samples, and the general
splendor that is our Code@ community for ExactTarget developershttps://code.exacttarget.com
.

Are there there any specific questions about our APIs which you may have
that could help encourage positive contributions? If you do, we'd welcome
you to ask those and stir up discussion on our Code@ Q/A Pagehttps://code.exacttarget.com/questions


Reply to this email directly or view it on GitHubhttps://github.com/ExactTarget/FuelSDK-Python/issues/8#issuecomment-25458117
.

Contributor

creatovisguru commented Oct 1, 2013

I appreciate your need for one-on-one attention. We have thousands of developers and I'd love to give them all 30 minutes...but that just isn't a realistic path forward for our organization (since it doesn't scale and I have to sleep).

Our company's leadership has invested in the development and curation of the Code@ community, we hold an annual conference with a developer track, and we offer valid support paths for our customers.

This allows us to create a healthy ecosystem. We're providing quality documentation and a very active community which has well over one thousand (1000) questions which have been asked and fewer than one percent are unanswered. (most of those answers are from our internal engineers and developer advocates)

I am sorry to hear that your previous experiences with Q/A boards have been unsuccessful. I hope through our interaction today, you will sign up for a Code@ account and join our community for a better experience.

jkbbwr commented Oct 2, 2013

https://code.exacttarget.com/question/rest-or-json I have asked my specific question, I wonder how long till there is a solution provided.

Contributor

cmoad commented Jan 20, 2014

Hi. I am taking a pass at cleaning up this library in my personal time. I am not affiliated with the Fuel team directly but do work at ExactTarget in marketing. I'm leaving this task open and I will try to address your concerns.

I did just merge a major refactor that turns this project into a true python module with a setup.py file. My hope is to submit to pypi in the near future once one other merge is completed.

In the spirit of open source I'll take all the help I can get. Thanks.

@cmoad cmoad was assigned Jan 20, 2014

@cmoad cmoad was unassigned by jkbbwr Dec 18, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment