ManageIQ / binary_struct

BinaryStruct is a class for dealing with binary structured data.
MIT License
4 stars 11 forks source link

Add support for Big and Little Endian modifiers #3

Closed jerryk55 closed 10 years ago

jerryk55 commented 10 years ago

This pull request also fixes encoding issues in existing tests caused by upgrading ruby from 1.8.7 to 1.9.3. @Fryguy, @chessbyte please review.

jerryk55 commented 10 years ago

It should be noted that this PR will force us to remove support for Ruby 1.8.7 and 1.9.2 in this Gem because the Big/Little Endian modifiers to Array#pack weren't added until Ruby 1.9.3. We should increment the Gem version and remove the older Ruby versions from the .travis.yml file. I'd like to get agreement as to version number before doing so - from @Fryguy and @jrafanie at the very least.

jrafanie commented 10 years ago

I'd be fine with this since both 1.8.7 and 1.9.2 are being EOL for "real" this time at the end of this month: https://www.ruby-lang.org/en/news/2014/07/01/eol-for-1-8-7-and-1-9-2/

But either way, we need to increment the VERSION file appropriately due to this breaking change.

jerryk55 commented 10 years ago

Agreed. Assuming we use Semantic versioning this would move the Gem from 1.0.1 to 1.1.0 since it is a backwards-compatible change (existing code should still work). I can modify that and the .travis.yml file.

Fryguy commented 10 years ago

backwards-compatible change (existing code should still work)

Is that true though...existing code (on 1.8.7) will break.

jerryk55 commented 10 years ago

Jason,

My implication is that any code that was working on 1.8.7 was using the Array#pack directives that did not include the ">" and "<" characters. Unless there were more directive characters in 1.8.7 that were removed in 1.9.3 (and I was not aware of that when I made my statement) I do not see why that code would no longer work in 1.9.3.

Jerry

On 7/16/14, 3:15 PM, Jason Frey wrote:

backwards-compatible change (existing code should still work)

Is that true though...existing code (on 1.8.7) will break.

— Reply to this email directly or view it on GitHub https://github.com/ManageIQ/binary_struct/pull/3#issuecomment-49212975.

Jerry Keselman Red Hat Virtualization R&D jerryk@redhat.com | p. 212 530 7873

Fryguy commented 10 years ago

Ok, I see that, but from a semver perspective, I think we have to cut a major version. This is one of those fine-line things :) In addition we may want to add the minimum Ruby version to the gemspec (but that can be done when we cut the new version).

Fryguy commented 10 years ago

You forgot to remove the gem version change :) If you could edit the old commit that might be cleaner.

Fryguy commented 10 years ago

Oh wait i see what I saw...yeah, let's amend the old commit instead of change and then change back :)

jerryk55 commented 10 years ago

Jason - in case we don't speak on your way to your desk :-) I'm not sure I"m getting you on this so lets talk....

coveralls commented 10 years ago

Coverage Status

Coverage decreased (-0.42%) when pulling 35b16ca60a055c4b5321af897ad879641c01f83c on jerryk55:endian_modifiers into c25d839547f34874449a323c1f36af9d3279fff2 on ManageIQ:master.

Fryguy commented 10 years ago

Seems that rbx-19mode is not a valid identifier: http://rubini.us/2013/12/03/testing-with-rbx-on-travis/

We can probably try with rbx-2

Fryguy commented 10 years ago

What I was referring to was that typically in a PR, when you're all done, the final patchset should just be the changes to implement the feature. There shouldn't be "fix typo" commits or a commit that changes something and then another commit that changes it back. In this case, the .travis.yml was changed, and then changed again, meaning that intermediate change is not relevant to the history of the project.

jerryk55 commented 10 years ago

Ah... Wish I'd read that before I had Joe R help me edit the last two commits so I wasn't changing the version.rb file. Oops.

Fryguy commented 10 years ago

No prob :smile: . It has to be modified for the rbx stuff anyway, so we can probably get that all cleaned up.

miq-bot commented 10 years ago

Checked commits https://github.com/jerryk55/binary_struct/commit/72636bdf8adf80e1c1c3006c1155bca17444db21 .. https://github.com/jerryk55/binary_struct/commit/89804ebaa2b6a8e2ad89a4c94e3f6b7ee1b3738a with rubocop 0.21.0 4 files checked, 4 offenses detected

spec/binary_struct_spec.rb

spec/endian_spec.rb

coveralls commented 10 years ago

Coverage Status

Coverage decreased (-0.52%) when pulling 89804ebaa2b6a8e2ad89a4c94e3f6b7ee1b3738a on jerryk55:endian_modifiers into c25d839547f34874449a323c1f36af9d3279fff2 on ManageIQ:master.