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

catch IO errors when writing media files, closes #4559 #4619

Merged
merged 1 commit into from
May 4, 2018
Merged

Conversation

danse
Copy link
Contributor

@danse danse commented May 2, 2018

if we do not catch these errors, any malformed entry in a media bag
could cause the loss of a whole document output. an example of
malformed entry is an entry with an empty file path.

for the error logging i looked for a way to preserve information from the IO error. An IO error like media/.: openBinaryFile: inappropriate type (Is a directory) can give helpful troubleshooting information to the users in case of errors in a path or problems with permission

@wilx
Copy link
Contributor

wilx commented May 3, 2018

@danse You need to fix the warning. See Travis CI output.

@danse
Copy link
Contributor Author

danse commented May 3, 2018

oops thanks for the heads up @wilx i fixed it let's wait for the new build

@jgm
Copy link
Owner

jgm commented May 3, 2018

Under these conditions, will we still get a CouldNotFetchResource warning, or just the IgnoredIOError?

I think it would be best if we still got CouldNotFetchResource as well.

@danse
Copy link
Contributor Author

danse commented May 3, 2018

Yes we still get the warning which is triggered by an independent part of the software, within fetchMediaResource if i remember correctly. Using the test file from #4559, the output with this change is the following

[WARNING] Could not fetch resource './ObjectReplacements/Object 1': replacing image with description
[WARNING] IO Error (ignored): media/.: openBinaryFile: inappropriate type (Is a directory)

@danse
Copy link
Contributor Author

danse commented May 3, 2018

the warning is triggered when the resource is not found. but then the ODT reader fetches the resource independently and stores an empty path in the media bag. when we try to use the path later on we get the error that is ignored in this change

@jgm
Copy link
Owner

jgm commented May 3, 2018 via email

@jgm
Copy link
Owner

jgm commented May 3, 2018

On reflection, not all IO errors will be pandoc bugs (a file might not have the right permissions, e.g.), so let's do this.

res <- liftIO $ tryIOError f
case res of
Left e -> report $ IgnoredIOError (E.displayException e)
Right _ -> pure ()
Copy link
Owner

Choose a reason for hiding this comment

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

Why not change type to IO a -> PandocIO a and change this line to

Right x -> return x

That makes the function more general.
Either that, or change the type to IO () -> PandocIO ().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IO a -> Pandoc a would require a default value when the operation fails, so i opted for IO () -> Pandoc (), i amended and rebased

if we do not catch these errors, any malformed entry in a media bag
could cause the loss of a whole document output. an example of
malformed entry is an entry with an empty file path
@danse
Copy link
Contributor Author

danse commented May 4, 2018

One could make a case for the view that this is a bug in pandoc, and should therefore lead to an error rather than a warning. Pandoc should handle this case better

I agree that this is a fault in pandoc, yet the conversion process can continue. Why would we want a fault to be fatal, rather than preserving what is unaffected? We could rename IgnoredIOError to RecoverableError, it might help us to keep in mind the idea. It could be shown to the user as IO Error: <description>, trying to continue anyway.... It sounds funny but it's clear. I agree that, if we had more logging levels, we would use the error level rather than the warning level for this one

@jgm jgm merged commit 59f0c1d into jgm:master May 4, 2018
@danse danse deleted the 4559 branch May 4, 2018 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants