search

iwanders / OBD9141

A class to read an ISO 9141-2 port found in OBD-II ports.
MIT License
232 stars 72 forks source link

Reorganize to comply with Arduino IDE 1.5 Library Specification #6

Closed adamvoss closed 7 years ago

adamvoss commented 7 years ago

Reorganize to comply with Arduino IDE 1.5 Library Specification

Specification can be found at https://github.com/arduino/Arduino/wiki/Arduino-IDE-1.5:-Library-specification.

Note: I just arbitrarily started the version at 1.0.0, since I did not see any existing version identifier.

iwanders commented 7 years ago

Thanks for putting up this PR.

Note: I just arbitrarily started the version at 1.0.0, since I did not see any existing version identifier.

Yeah, I don't really bother with version numbers for my own libraries, I usually embed the git hash into my firmware at compile time (like this). I usually have the libraries as a git submodule, which are at a specific commit for the parent repository's commit. So that way I can always discern which version is running if.

Using version 1.0.0 or higher means that the public API should be considered stable. Which I guess we can just say it is, I have used a project that relied on this library almost daily for over a year and didn't run into issues / bugs. Since I don't have access to an OBD9141 port right now I certainly won't be changing the public API. Judging from the faq entry we are definitely past 1.0.0!

adamvoss commented 7 years ago

All your suggestions sounded good. I did a force-push that should addressed them.

iwanders commented 7 years ago

Thanks for your contribution.

iwanders commented 7 years ago

Was the zip file intentional? I didn't spot that in the first version of the PR... It also contains a .git folder, but the repository in the zip file itself also has unstaged changes.

I presume this was included by accident (and I didn't spot that on the second review pass). Am I good to delete that zipfile?

adamvoss commented 7 years ago

Darn, I'm really sorry about that! Yeah, I created that for testing the library.properties file and it it accidentally was added when I did the fixes.

As far as I'm concerned, you could even go ahead and re-write to get rid of it even if you are concerned about repo size and such.

iwanders commented 7 years ago

No problem, mistakes happen. Next time its better to add additional commits to the PR, and then they can be squashed (if need be) just before the merge.

As far as I'm concerned, you could even go ahead and re-write to get rid of it even if you are concerned about repo size and such.

Even though that's not the prefered way to rewrite the history on the master branch once it has gone public I've done just that. Given that the other version of the branch was out there for just 5 minutes I think it's highly unlikely that someone already pulled the changes and has now ended up with a diverged repository. So I deleted the zip file, ammended the merge commit and force pushed the changes. Thanks for the quick response to resolve this.