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

#10344 fix #10345

Closed
wants to merge 2 commits into from
Closed

#10344 fix #10345

wants to merge 2 commits into from

Conversation

Voileexperiments
Copy link

ref: #10344

Iterate all keys in infoDict instead of all keys in DocumentInfoValidators which infoDict has the same key of, so as to not ignore the custom info dict entries.

ref: #10344

Iterate all keys in `infoDict` instead of all keys in `DocumentInfoValidators` which `infoDict` has the same key of, so as to not ignore the custom info dict entries.
linter complains line length >80 (83)
@@ -545,16 +545,15 @@ var PDFDocument = (function PDFDocumentClosure() {
}
if (isDict(infoDict)) {
// Only fill the document info with valid entries from the spec.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This patch is very clearly breaking the assumption listed above, which exists to prevent errors since not all data structures (one example is Dict) can be sent from the worker to the main thread.

Assuming first of all that this is even desired functionality, the patch unfortunately cannot be accepted in its current form (furthermore there's also the issue of no tests being include here).

Copy link
Author

Choose a reason for hiding this comment

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

Okay, then what is the intended functionality? Info dict is clearly in the specs since every PDF accepts custom info dict entries. I do not see any discussions among the issues about this matter.

@timvandermeij
Copy link
Contributor

Closing in favor of the more complete solution in #10346.

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

Successfully merging this pull request may close these issues.

3 participants