Skip to content
This repository has been archived by the owner on Jul 7, 2020. It is now read-only.

Minor fixes and cleanup #1

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Minor fixes and cleanup #1

wants to merge 5 commits into from

Conversation

josharian
Copy link
Collaborator

No description provided.

Josh Bleecher Snyder added 5 commits September 22, 2014 16:40
Generated by stringer.
Encountered parsing NYT crosswords.
Restoring from an empty graphics stack looks like
a mistake in the PDF, but other PDF readers
handle it fine, and ignoring it appears to
work correctly.

Encountered parsing NYT crosswords.
Some PDFs contain non-whitespace junk after the
%%EOF marker. Other PDF parsers handle it fine.

Encountered parsing NYT crosswords.
Copy link
Contributor

@odeke-em odeke-em left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And at the end, please squash the commits into one :)

}
return c
default:
panic("bad content kind")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this panic here might cause undocumented and unexpected problems for users. I was using the package earlier on the bus back from school and wrote code that invoked p.Content() without explicitly checking the Kind(). Perhaps we can just make an explicit check for Array then let the contentForStream handle be the catch all?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there other known cases? Would contentForStream handle them correctly?

It's been ages since I've looked at this code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question @josharian: I am not sure, I am not well versed with the PDF spec but I thought it was a little unexpected to panic on other types. So far I've only seen mentions of Stream and Array for "Content" in the spec.
Perhaps: I think it would handle the "other cases" properly because it contains the almost the same code before this patch, and that has worked in the past.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or it was silently returning junk before this patch. I'd rather be explicit, all else being equal.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, yap that makes sense to me now.

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

Successfully merging this pull request may close these issues.

2 participants