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

[INFRA] Auto adjust table fences before PDF conversion #560

Merged

Conversation

sebastientourbier
Copy link
Collaborator

This PR is dedicated to correct automatically the dashes and align table fences before generation of the PDF.
It takes over PR #553 (where the intention was a manual fix) and is dedicated to close #549

@sebastientourbier
Copy link
Collaborator Author

sebastientourbier commented Aug 4, 2020

@sappelhoff I opened this PR to address automatically the table alignment issue in PDFs.
I implemented two functions in the pandoc_script.py that correct and rewrite the markdown files before the generation of the PDF. At this stage, this should fix most all tables in the generated PDF. Just the build_docs_pdf job fails as I had to use numpy which should be install on circleci. What would be the best way?

@sappelhoff
Copy link
Member

sappelhoff commented Aug 4, 2020

this should fix most all tables in the generated PDF

awesome!

What would be the best way?

you can add numpy to https://github.com/bids-standard/bids-specification/blob/master/requirements.txt

let me correct that. Look at these lines in .circleci/config.yml:

- run:
command: |
pip install --upgrade pip

you add the same syntax in the build_docs_pdf job, and use pip to install numpy

That is, insert the following lines:

      - run:
          command: |
            pip install --upgrade pip
            pip install numpy

directly after these lines:

steps:
- checkout:
path: ~/bids-specification

pdf_build_src/header.tex Outdated Show resolved Hide resolved
Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

I see a lot of good progress here! Some remarks below.

pdf_build_src/process_markdowns.py Outdated Show resolved Hide resolved
pdf_build_src/process_markdowns.py Outdated Show resolved Hide resolved
pdf_build_src/process_markdowns.py Outdated Show resolved Hide resolved
pdf_build_src/process_markdowns.py Outdated Show resolved Hide resolved

# Offset that can be used to ajust the correction of number of dashes in the first and
# second columns by the number specified
offset = [15, 5]
Copy link
Member

Choose a reason for hiding this comment

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

where do these numbers come from?

Copy link
Collaborator Author

@sebastientourbier sebastientourbier Aug 5, 2020

Choose a reason for hiding this comment

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

This is indeed really not trivial to readjust the tables and these offsets are very useful in adjusting columns where a few or a lot of characters are present. This would be ideal to get rid of them but I have not ended up with a golden rule for the moment. Any idea?

pdf_build_src/process_markdowns.py Outdated Show resolved Hide resolved
pdf_build_src/process_markdowns.py Outdated Show resolved Hide resolved
Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

some nitpicks with the docstrings, but I think that style is important and that's how it's done in many big python packages. :-)

pdf_build_src/process_markdowns.py Outdated Show resolved Hide resolved
pdf_build_src/process_markdowns.py Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@sebastientourbier sebastientourbier left a comment

Choose a reason for hiding this comment

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

@sappelhoff Seems at this point, tables are properly rendered in the PDF and all suggestions that you made have been incorporated 👍

@sappelhoff
Copy link
Member

sappelhoff commented Aug 5, 2020

The entity table still seems a bit buggy, do you know why? --> https://3448-150465237-gh.circle-artifacts.com/0/bids-spec.pdf

I see now why the entity table is still buggy :-) --> I think you can fix that one as well, because with the automatic script we don't run into the problems I anticipated earlier when you were still doing manual fixes.

also, the "submultiples" in the units section (appendix VI)

Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

One more review - note that I have made several code suggestions. You can directly merge them from the GitHub GUI using these buttons (screenshot):
image

just make sure that after you have done that, do a git pull before continuing to work locally on the code, otherwise you will create a merge conflict.

pdf_build_src/process_markdowns.py Outdated Show resolved Hide resolved
pdf_build_src/process_markdowns.py Outdated Show resolved Hide resolved
pdf_build_src/process_markdowns.py Outdated Show resolved Hide resolved
pdf_build_src/process_markdowns.py Outdated Show resolved Hide resolved
pdf_build_src/process_markdowns.py Outdated Show resolved Hide resolved
pdf_build_src/process_markdowns.py Outdated Show resolved Hide resolved
pdf_build_src/process_markdowns.py Outdated Show resolved Hide resolved
pdf_build_src/process_markdowns.py Outdated Show resolved Hide resolved
pdf_build_src/process_markdowns.py Outdated Show resolved Hide resolved
sebastientourbier and others added 8 commits August 5, 2020 18:13
Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
@sebastientourbier
Copy link
Collaborator Author

@sappelhoff I created the functions as suggested and re-factorized the code accordingly. I also removed 04-entity-table.md from the exclude_files list. This however do not fix the entity table. I think the main problem here is a number of 16 columns is far too big to correctly render this table in portrait layout. A solution could be to adopt a paysage layout but I guess for that we would need to edit the latex file generated by pandoc before it builds the PDF. Not sure if it is possible and/or how to do. @Arshitha any idea?

@sebastientourbier
Copy link
Collaborator Author

@sappelhoff Seems my suggestion #562 (comment) (PR #562) works to fix #551 regarding emojis.
See the generated PDF: https://3545-150465237-gh.circle-artifacts.com/0/bids-spec.pdf
I will revert my changes here thought to not create a mess.

Copy link
Collaborator Author

@sebastientourbier sebastientourbier left a comment

Choose a reason for hiding this comment

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

@sappelhoff This should resolve all remaining opened concerns 👍

pdf_build_src/process_markdowns.py Show resolved Hide resolved
@sappelhoff sappelhoff changed the title [FIX] Table fences in PDF of specification misaligned (Auto) [INFRA] Table fences in PDF of specification misaligned (Auto) Aug 8, 2020
@sappelhoff
Copy link
Member

@sebastientourbier do you know why from dd6545e onwards the diff is completely unreadable?

Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

I checked the output and only one table was screwed up (and the entity table, but that's fine).

for that single table, I submitted a fix in adbde72

Overall only 3 minor style comments, then I think we can merge this.

pdf_build_src/process_markdowns.py Outdated Show resolved Hide resolved
pdf_build_src/process_markdowns.py Outdated Show resolved Hide resolved
pdf_build_src/process_markdowns.py Outdated Show resolved Hide resolved
@sappelhoff sappelhoff changed the title [INFRA] Table fences in PDF of specification misaligned (Auto) [INFRA] Auto adjust table fences before PDF conversion Aug 8, 2020
sebastientourbier and others added 3 commits August 10, 2020 08:57
Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

great :-) Thanks a lot @sebastientourbier

@sappelhoff sappelhoff merged commit 7f1f61f into bids-standard:master Aug 10, 2020
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.

Table fences in PDF of specification misaligned
2 participants