Add support for Big and Little Endian modifiers #3
Conversation
Add the two endian modifiers '>' and '<' specifying that the directive should be interpretted as Big Endian or Little Endian, respectively. These modifiers will work only for the following directives: I i L l Q q S s Attempting to use the endian modifiers on other directives will result in an exception being raised.
1. Add support for the big and little endian modifier characters '>' and '<', respectively. Add the endian_spec.rb to test this new support. 2. Fix binary_struct_spec.rb and gif_spec.rb tests that have been failing since the ruby upgrade from 1.8.7 to 1.9.3 due to issues with the binary encoding of strings.
1. Remove a warning caused by reusing a constant name in two spec files. 2. Fix existing binary_struct test force_encoding twice to be consistent in redundant case.
|
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 |
|
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. |
|
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. |
| STRING_FORMATS = %w{A a M m u} | ||
| STRING_FORMATS = %w(A a M m u) | ||
| ENDIAN_FORMATS = %w(I i L l Q q S s) | ||
| ENDIAN_MODIFIERS = %w(> <) |
Is that true though...existing code (on 1.8.7) will break. |
| - "1.9.3" | ||
| - "2.0.0" | ||
| - "2.1.0" |
There was a problem hiding this comment.
To allow us to keep up with patch level updates, can you make these 1.9.3, 2.0, and 2.1?
There was a problem hiding this comment.
Yup.
On 7/16/14, 3:47 PM, Jason Frey wrote:
In .travis.yml:
- "1.9.3"
- "2.0.0"
- "2.1.0"
To allow us to keep up with patch level updates, can you make these
|1.9.3|, |2.0|, and |2.1|?—
Reply to this email directly or view it on GitHub
https://github.com/ManageIQ/binary_struct/pull/3/files#r15022746.
Jerry Keselman
Red Hat Virtualization R&D
jerryk@redhat.com | p. 212 530 7873
|
Jason, My implication is that any code that was working on 1.8.7 was using the Jerry On 7/16/14, 3:15 PM, Jason Frey wrote:
Jerry Keselman |
|
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). |
|
You forgot to remove the gem version change :) If you could edit the old commit that might be cleaner. |
|
Oh wait i see what I saw...yeah, let's amend the old commit instead of change and then change back :) |
|
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.... |
Change the .travis.yml file so that Travis no longer runs the tests against Ruby 1.8.7 and 1.9.2 since the Endian modifiers were not added to Array#pack until 1.9.3. While we're add it add Ruby 2.0.0 and 2.1.0 to the file to run the tests against the newer Rubies.
|
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 |
|
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. |
|
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. |
|
No prob 😄 . It has to be modified for the rbx stuff anyway, so we can probably get that all cleaned up. |
1. Remove jruby & rubinius 1.8 statements from .travis.yml and add rubinius 2. 2. Undo attempts to fix Rubocop "void context" warnings which result in redundant code in tests. 3. Simplify validate_definition_entry_modifier & rename to valid_definition_entry_modifier? since it doesn't raise an exception 4. Simplify get_size 5. Align assignments in constant definitions when possible. 6. Remove unnecessary "()" in new lambda syntax.
|
Checked commits jerryk55@72636bd .. jerryk55@89804eb with rubocop 0.21.0 spec/binary_struct_spec.rb spec/endian_spec.rb |
Add support for Big and Little Endian modifiers
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.