Skip to content
This repository has been archived by the owner on Aug 14, 2022. It is now read-only.

Implementing open-pdf support. #24

Closed
wants to merge 1 commit into from
Closed

Conversation

boborbt
Copy link
Contributor

@boborbt boborbt commented Aug 2, 2014

Presently the spec only tests for correct handling of errors. Do not really have idea what to test in case of success. The problem is: if I stub out the exec function there is nothing to test, otherwise the system will launch Preview (which I assume is not what we want).

Other issues with specs: should the PreviewAppPdfOpener spec be run only on systems that support that program? What's the idiomatic way of solving this problem? A check in the spec itself?

@boborbt
Copy link
Contributor Author

boborbt commented Aug 2, 2014

Here is a first version of the pdf-opener. exec is async so I had to include some call-backs for finishing and error handling. I've gone with the "standard" error/next pattern.

@thomasjo
Copy link
Owner

thomasjo commented Aug 4, 2014

Just merged and pushes the log-parsing branch into master. Just rebase your branch on top of the new changes; git rebase master (after you've synced your master branch with mine).

Looks like you haven't touched any of the existing files, so you should have no conflicts 😄

I added a simple console.log statement inside of the showResult function, so feel free to replace that with your invokation of PDF opening magic 👯

@boborbt
Copy link
Contributor Author

boborbt commented Aug 4, 2014

I did the rebasing and then force-pushed the changes onto my remote branch. I'll now start to look into integrating the open-pdf classes into the latex.coffee.

Presently the spec only tests for correct handling of errors. Do not really have idea what to test in case of success.
@boborbt
Copy link
Contributor Author

boborbt commented Aug 4, 2014

I've implemented OS checking so that it does not crashes if the OS is not supported.
Also, I've squashed all commits into a single one.

@thomasjo
Copy link
Owner

I applied your pull request to a new branch, and then made some changes in 14641e6. My goal was to simplify the code a little. Let me know if you think it looks OK, or perhaps you feel I changed too much?

Look forward to hearing your thoughts. And thanks again for contributing, I really appreciate all your efforts!

@boborbt
Copy link
Contributor Author

boborbt commented Aug 10, 2014

I've gone through your changes and I think they improve the overall organisation. I tend to overgeneralise designs and I think you rightfully tried to keep things simpler. The one change I'm not sure about is the removal of the (admittedly horribly named) PdfOpeners class. Right now, the additional clutter in latex.coffee does not matter, but once more OS's and user preferences will be supported I think this may change. As I was saying, though, right now the changes do simplify things, so it may be worthwhile to keep the things as they are and refactor the method when necessity arises.

As a stylistic note: I find
exitCode = error?.code || 0
much more readable then
exitCode = error?.code ? 0.
May it be a ruby bias and a lack of acquaintance with coffeescript idioms on my part?

P.S.: I did not investigate why, but the time taken to run the full test suite is an order of magnitude higher in the new branch (~5s vs ~0.5s).

@thomasjo
Copy link
Owner

Merged in 61d8a97.

Reason for the test suite taking longer is that the relevant specs now run against real latexmk, not just a fake. This change was necessary to ensure end-to-end functionality is working as intended across platforms.

@thomasjo thomasjo closed this Aug 10, 2014
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