Added the option to specify the VIN number to Update_cars #3
Conversation
…ate the one vehicle
There was a problem hiding this comment.
Overall, great feature to have, thanks for submitting it.
I'd rather do this on the constructor as explained inline so that the rest of the logic can more or less stay the same. It seems like a cleaner pattern that if people only want partial results, they'll always want those and not have to juggle the vin numbers in their own code after the constructor.
If you respin it that way, I'll be happy to merge and release. And bonus points if you add a test in the process.
| @@ -74,6 +74,9 @@ Usage is very basic. | |||
| # update stats for all cars, and returns the list of cars. This will take | |||
| # 60+ seconds per car | |||
| cars = page.update_cars() | |||
|
|
|||
| # Optionally you can call update_cars() with a vin of your choice to only update that car. | |||
| cars = page.update_cars('vin number') | |||
There was a problem hiding this comment.
I think that instead of putting the special logic on the update_cars to take vin, probably just make vin optional to the MyChevy constructor. Then get_cars() will just filter out the list of cars that don't match the vin, and should throw an exception if no car was found. That will let update_cars() remain unchanged, as the loop will just be a single car element.
There was a problem hiding this comment.
Oh I like that idea better. I will look into making the change to the constructor. I will also try to create a unit test for it too and then submit a new pull request.
| @@ -110,7 +111,7 @@ def from_json(self, data): | |||
| for a in CAR_ATTRS: | |||
| setattr(self, a, d[a]) | |||
|
|
|||
| except json.JSONDecodeError: | |||
| except ValueError: | |||
There was a problem hiding this comment.
Yeh, JSONDecodeError was added in python 3.5, we can roll it back to value error for earlier versions. Probably also add 3.4 back into travis testing.
There was a problem hiding this comment.
Looking at the Travis tests I wasn't aware that this was developed with Python3. I have been doing all of my development and runs with Python2. Since Python2 isn't tested it might be nice to mention Python3 either in the README or put a sys.version_info[0] check in there to warn users that Python2 is unsupported.
There was a problem hiding this comment.
Ah, good idea. I had the trove information up to date, but we could throw a warning there as well.
This change adds the option to update_cars() to supply a VIN so that only the VIN number specified is updated.
I also changed the Exception in "from_json()" function.
I was getting
AttributeError: 'module' object has no attribute 'JSONDecodeError'According to this StackOverflow JSONDecodeError inherits ValueError.
https://stackoverflow.com/questions/8381193/python-handle-json-decode-error-when-nothing-returned
This change is