Skip to content

Issue #29 Added describeLead rest end point to the library#30

Open
ghost wants to merge 2 commits intomasterfrom
unknown repository
Open

Issue #29 Added describeLead rest end point to the library#30
ghost wants to merge 2 commits intomasterfrom
unknown repository

Conversation

@ghost
Copy link

@ghost ghost commented Mar 7, 2016

Please review changes added as part of issue #29.

Thank you.

@Jaesin
Copy link

Jaesin commented Oct 5, 2016

This is also in #34 but by a different name getFields. Is describeLead more desirable?

@ghost
Copy link
Author

ghost commented Oct 5, 2016

Due to the snail pace on PR approvals with this project we were forced to use a forked local branch on our platform. I intend to periodically check back and merge new features into our local repo.

@itmayziii
Copy link

itmayziii commented Oct 10, 2016

@Jaesin In this case getFields to me would be more desirable with some tweaking to how it was implemented in #34.

Currently the getFields method only supports leads. In fact this whole repository only supports Leads. With the idea of expanding this client to support companies and opportunities we should think of how we want to move forward with naming.

Here are the options in my opinion.

Option 1:
Have a separate function call for each object in Marketo.

  1. describeLeads
  2. describeCompanies
    3 describeOpportunities

OR...

Option 2:
Have 1 method call that takes the object name as a parameter
describeObject($objectName) or getFields($objectName)
Marketo uses the word "describe" so it seems to make sense to name our functions that way.

If you really want to have the method names describe what is happening then you could just wrap the general describeObject call with your named function

describeLeads() { return $this->describeObject('Leads'); }

That way you still have all the code being reused with describeObject but you get the nice method name as well. Just my thoughts, I plan on contributing to this repo soon with functionality to support companies and opportunities so I wanted some others opinions on how to move forward.

@dchesterton Any thoughts?

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