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

Pass --no-wrap to pandoc #145

Merged
merged 3 commits into from Jan 4, 2018

Conversation

@jimhester
Copy link
Member

commented Dec 21, 2017

Fixes #140

@jennybc

This comment has been minimized.

Copy link
Member

commented Dec 21, 2017

Thanks! This is a better solution than I had brewing.

I'm going to hold off on merge until I can sort out why all hell broke loose on travis/appveyor.

@jimhester jimhester force-pushed the jimhester:ad_linebreaks branch from 7bd5948 to 6c5cce4 Dec 21, 2017

@jimhester jimhester changed the title Use non-breaking spaces so the ad is not wrapped on GitHub Pass --wrap=preserve to pandoc Dec 21, 2017

@jimhester

This comment has been minimized.

Copy link
Member Author

commented Dec 21, 2017

I think passing --wrap=preserve is actually a better fix than what I originally proposed, however from the travis failures it looks like it is a new option.

@jimhester

This comment has been minimized.

Copy link
Member Author

commented Dec 21, 2017

I will figure out what the old option would be.

@jimhester

This comment has been minimized.

Copy link
Member Author

commented Dec 21, 2017

So --wrap=preserve was added in pandoc 1.16, but travis is currently only using pandoc 1.15.

So we will have to use --no-wrap for backwards compatibility I guess, unless you want to condition on rmarkdown::pandoc_version()

Use no-wrap instead of --wrap=preserve
The latter is only available on pandoc versions >= 1.16
@jimhester

This comment has been minimized.

Copy link
Member Author

commented Dec 21, 2017

Well that fixed the errors on travis, but appveyor has a bunch of failures, it is using pandoc v1.13.1 but it looks like --no-wrap was added very early to pandoc so not sure why they are failing.

@jennybc

This comment has been minimized.

Copy link
Member

commented Dec 21, 2017

I have suffered from messing with other pandoc options in the past and had to back out. @jimhester How gross do you find my suggestion in #140 to deal with this post-render?

@jimhester

This comment has been minimized.

Copy link
Member Author

commented Dec 21, 2017

pretty gross, I really think the pandoc option is the right fix, I can try to hunt down what is going on in appveyor tomorrow.

@jennybc

This comment has been minimized.

Copy link
Member

commented Dec 21, 2017

@jimhester How about this? All the results aren't in yet ...

https://github.com/tidyverse/reprex/pull/146/files

Update: well that just shifts the problem elsewhere in the ad.

@jimhester

This comment has been minimized.

Copy link
Member Author

commented Dec 21, 2017

Yes, using non breaking spaces was my original fix. I really think the pandoc option is the right thing to do.

@jennybc

This comment has been minimized.

Copy link
Member

commented Dec 22, 2017

The older, now deprecated way to say this in pandoc is --no-wrap. But that does not immediately clear things up on AppVeyor. I haven't delved into this yet at all, though.

@jennybc

This comment has been minimized.

Copy link
Member

commented Dec 22, 2017

Oh, sorry, I now see you had already done this and gotten same result.

@krlmlr krlmlr referenced this pull request Dec 22, 2017

Closed

Don't wrap GitHub reprexes #147

@jimhester jimhester changed the title Pass --wrap=preserve to pandoc Pass --no-wrap to pandoc Dec 22, 2017

Use the proper pandoc version to prevent line wrapping
Prior to 1.16 this is --no-wrap, after it is --wrap=preserve

This should fix the appveyor failures, it works fine on a windows VM
with this version of pandoc.

@jimhester jimhester force-pushed the jimhester:ad_linebreaks branch from c6d8f0c to b0c06c5 Dec 23, 2017

@jimhester

This comment has been minimized.

Copy link
Member Author

commented Dec 27, 2017

In c6d8f0c only one test fails on Appveyor, perhaps unrelated to these changes, so I think this is fixed.

@jimhester

This comment has been minimized.

Copy link
Member Author

commented Jan 4, 2018

@jennybc does the current state of this PR look good to you? I am pretty confident this is the best way to fix this.

@jennybc

This comment has been minimized.

Copy link
Member

commented Jan 4, 2018

Yes, thanks!

@jennybc jennybc merged commit 6b78062 into tidyverse:master Jan 4, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.