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

reading DOCX, pick table width from the longest row -- closes #4360 #4361

Closed
wants to merge 1 commit into from

Conversation

danse
Copy link
Contributor

@danse danse commented Feb 12, 2018

this is related to jgm/pandoc-types#36. the test here will have to be updated when that version of pandoc-types will made it to pandoc

@jkr
Copy link
Collaborator

jkr commented Feb 13, 2018

This looks good, except that ghc 7.10 build fails on the NonEmpty import, since semigroups wasn't in base yet. We have to either load semigroups for earlier ghc or, what's probably less trouble at the moment, keep the ugly case statement at 654. I'd recommend the latter.

@danse
Copy link
Contributor Author

danse commented Feb 14, 2018

cool, i propose a simple ad hoc implementation of nonEmpty, let me know what you think. cheers

@jkr
Copy link
Collaborator

jkr commented Feb 15, 2018

Sure. When we drop support for ghc 7.x, we can use real NonEmpty. Couple more things:

  1. The test case doesn't demonstrate the issue, or the fix. Since the first row is the longest one, it gets the same result with or without the commit.

  2. The commit message could be more helpful. Could you commit with a message like:

    Docx reader: Pick table width from the longest row
    
    [Longer message explaining change]
    
    closes: #4360
    

    That way, when people see the commit in the future, they'll understand it.

This change is intended to preserve as much of the table content as
possible

Closes jgm#4360
@danse
Copy link
Contributor Author

danse commented Feb 15, 2018

actually the test case is suited and will fail if the fix is not there. what looks like the first row in the native are actually the headers, but pandoc would use the length of the first row before this change. i updated the commit message as you asked

@jkr
Copy link
Collaborator

jkr commented Feb 15, 2018

Oh, I see -- I misunderstood the purpose of the commit. I apologize. (But it does underscore the importance of informative commit messages.) This change only affects the alignments and column widths only, correct?

Thanks for the fix, and your patience!

@jkr
Copy link
Collaborator

jkr commented Feb 15, 2018

committed in e6ff7f7

@jkr jkr closed this Feb 15, 2018
@danse danse deleted the 4360 branch February 15, 2018 22:15
@danse
Copy link
Contributor Author

danse commented Feb 15, 2018

thanks to you 👍

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.

None yet

2 participants