Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for issue #13 (lots of ruby warnings generated). #21

Merged
merged 3 commits into from
Sep 10, 2014
Merged

Fix for issue #13 (lots of ruby warnings generated). #21

merged 3 commits into from
Sep 10, 2014

Conversation

jessedoyle
Copy link
Contributor

Many of TTFunk's classes had unused variable assignments. The warnings were bothering me, so I changed all unused variables to underscores, but left a comment above the change outlining what the variable represents. All tests pass for me and the warnings are no longer generated.

@jessedoyle jessedoyle mentioned this pull request Sep 9, 2014
@jessedoyle
Copy link
Contributor Author

I wasn't sure what to do with with the parse_format! method in the file ttfunk/lib/ttfunk/table/post/format25.rb on line 17.

The code was (number_of_glyphs was unused):

def parse_format!
  number_of_glyphs = read(2, 'n').first
  @offsets = read(@number_of_glyphs, "c*")
end

I'm guessing the original author intended to set the instance variable @number_of_glyphs instead of the local variable number_of_glyphs? Or maybe I'm missing something?

Anyways I just removed the unused variable assignment and kept the read, which should theoretically behave in the same manner.

@practicingruby
Copy link
Member

@jessedoyle You may have uncovered a bug, because @number_of_glyphs does not appear to be defined anywhere else in the project. For now let's call read(number_of_glyphs, "c*"), but I"m going to open another ticket to try to confirm the correct behavior.

@practicingruby
Copy link
Member

@jessedoyle I've taken a closer look at this and I'm pretty sure it's a bug. Because the post table format 2.5 was deprecated back in February 2000, I think we may want to just stop supporting it in TTFunk for the moment. This feature probably never worked properly, so I don't think it'd hurt anyone to turn it off entirely. I'll take care of that in a separate commit after merging your code.

Thanks for the great work!

practicingruby added a commit that referenced this pull request Sep 10, 2014
Fix for issue #13 (lots of ruby warnings generated).
@practicingruby practicingruby merged commit 2de046c into prawnpdf:master Sep 10, 2014
@practicingruby
Copy link
Member

@jessedoyle Because your pull request was accepted, you now have commit access to all prawnpdf repositories. See the contribution guidelines for more details, and thanks for the patch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants