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

Convert ruby-pardot to use JSON API instead of XML #5

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

artificialrobot
Copy link

Request JSON responses and update tests to reflect the JSON API
Changed POST request parameters from query params to body params to allow for long parameter values
Created Pardot Base Object to remove duplicate code
Fix the format argument to be json and set the output param to simple
Updated to use https rubygems
Updated gems

Request JSON responses and update tests to reflect the JSON API
Fix the format argument to be json and set the output param to simple
@dcunning
Copy link
Contributor

I'm not the manager of this gem, but I'll weigh in nevertheless. This changeset is hard to follow with a number of changes all combined into one commit:

  • Using the bundler-generated-style Gemfile instead of the older gemcutter. I see no problem with this change, but it has nothing to do with JSON vs XML
  • Changing all double-quotes to single-quotes. Adds a bunch of changed lines that distract and hide the functional changes
  • Change the gem to use the JSON API instead of the XML API, but I'd like to know (other than JSON being preferred over XML in popular circles) why this is worth doing. The gem should hide the implementation details of the Content-Type right?

@stephenreid
Copy link
Contributor

I'm not seeing official documentation that Json is supported on http://developer.pardot.com should I be looking for it somewhere?

@stephenreid
Copy link
Contributor

To agree with @dcunning, @artificialrobot can you break this pull request into smaller commits so that it can be reviewed better? Feature specific pulls will allow better history as well

  • Double Quotes to Single Quotes is a good thing as it will save some memory and processing time.
  • JSON is preferred over XML because the payload is slightly smaller and it should be able to be processed more quickly on the client. I'm not sure, but based on prior knowledge of the Pardot Api, I would suspect that server generation of JSON could actually take longer.

@artificialrobot
Copy link
Author

Hey guys, I will see what I can do to break up this PR sorry for munging it all into one, I started down one road of adding support for JSON and ended up making the other stylistic changes as I went which I agree does make this more confusing as a single diff, but made sense as I was changing. Thanks for the feedback. If you would like to just close this PR and I can rebase and break up the commits in #6 so that there are separate commits for the various changes that would probably be do-able.

It may be a bit before I can do this work as I have left the job where I was working with Pardot, but if you think these changes are worthwhile then I will be glad to get it in better shape.

@stephenreid
Copy link
Contributor

Hey @artificialrobot. Thank you for contributing and getting back to us. I talked with someone at @ pardot and it sounds like they will need to get some SalesForce paperwork to allow their management of the project.

@znbailey @afischoff Can one of you push the paperwork through on this? and then check out the pull requests?

@dcunning
Copy link
Contributor

Double Quotes to Single Quotes is a good thing as it will save some memory and processing time.

Do you have anything to support this? Performance comparison thread 1 and thread 2.

I've always placed it in the category of "style preference"

@stephenreid
Copy link
Contributor

Rubocop seemed to support this theory.

It's the same as the PHP world. "This is a $string" processes while 'this is a $string'

The conclusion from most testing though is that finding out if a string has a couple of #{}s in it doesn't take a measurable amount of time (somewhat dependent on length or number of processible characters)

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.

None yet

3 participants