Skip to content

Full decoding now pulls more data#3

Open
fabrouy wants to merge 7 commits intoSovietaced:masterfrom
fabrouy:master
Open

Full decoding now pulls more data#3
fabrouy wants to merge 7 commits intoSovietaced:masterfrom
fabrouy:master

Conversation

@fabrouy
Copy link
Copy Markdown

@fabrouy fabrouy commented Jul 28, 2015

Hey Jason,

I wrote a couple lines. My idea was to return more data when you fully decode a vin number.

  • I created a transmission and drivetrain class.
  • I updated engine class with more attributes.
  • I fixed how colors where pulled (it was only pulling first two array positions) and made a couple changes regarding how the object gets constructed. I wanted to make it easy to access interior and exterior colors.
  • Had a similar approach for options.
  • I commented out the AS dependency (could not make it work with it, is it necessary?)
  • I updated the exception class, I think keys where wrong.
  • Updated sanitization. It failed for me when tried to basic decode because some of the params (that are passed to the request object when full decoding) where not initialized.

Tests for this are missing, but check it out, let me know what you think.

Regards,

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fabrouy Sorry for the dumb question but is this the result of a more recent API change?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if it was different in the past. Right now doing .map over colors just loops over a couple of keys (interior and exterior) not the actual colors.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I'm fine with that as long as the test works out fine.

@Sovietaced
Copy link
Copy Markdown
Owner

@fabrouy I'm finally taking a look at this now. Let me see if I can get my local dev environment going..

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so this is used in the request.rb class

require 'active_support/core_ext/hash'

When I bundle it fails to install but I know I have it... I'm confused.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed this up on master.

@Sovietaced
Copy link
Copy Markdown
Owner

@fabrouy Once you fix the merge conflicts, you should be able to run all the tests locally! Things are no longer broken as the build is passing.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks reasonable.

@Sovietaced
Copy link
Copy Markdown
Owner

If you add another commit, travis ci should build the pull request.

@Sovietaced
Copy link
Copy Markdown
Owner

@fabrouy Do you want me to take over this PR?

@fabrouy
Copy link
Copy Markdown
Author

fabrouy commented Nov 26, 2015

Sure! Very bussy lately unfortunately.

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