pdf version specs #427

Arshitha · 2020-02-25T20:29:48Z

This is a PR almost identical to PR #400 updated-pdf-version-specs except that this has much cleaner commit history and was created to address the issue of unwanted commits in the above referenced PR.

To gain a better understanding of why certain decisions were made while fixing the issue of generating pdf version of the specs, discussions in the following issues would be useful references:

pdf version of spec pdf version of spec #135
updated-pdf-version-specs Updated pdf version specs #400

Bugs Fixed:

Stable Table of Contents page
Cover page
Non-tabular text overflow
Overlapping columns of tables (owing to small widths of the pipe tables in the markdown files)
- Markdown files were edited (one-time edit) to increase the column widths of pipe tables
- Does not affect the read the docs version of specs. However, if any tables were added to the specs in future, there needs to a standard that works for both pdf AND read the docs version of the specs.
Automating header to include the latest release version number and build date (pulled from mkdocs.yml)

Enhancements required (non-exhaustive list):

Internal links referencing either the same markdown file or other markdown files break during conversion from markdown to pdf. Currently, all of the internal links are removed and replaced by the accompanying text.
Superscripts are lost in conversion as well
Tried a couple different family of fonts to fix the missing character warnings for emojis used in the markdowns with no success. It would be ideal to fix this to match the Read The Docs version more closely
Entity table in Appendix IV has overlapping unreadable columns since it's markdown equivalent has more than 10 columns. Refer to issue for more discussion

updated pandon-script.sh adds a - a cover page. cover.tex file uses the bids logo - a header with BIDS version number, however, this isn't automated. The header.tex file needs to be updated for new version release of BIDS specs. - font change is possible. However, the default LaTex font seemed better to me but can be changed with -V mainfont="DejaVu Serif" or any other font. - spacing after 'Quantitative T1rho brain imaging' and before URL on page 21 was modified in the corresponding .md file in the src directory.

merging changes from base repo

- automated header extraction from changelog - removing all internal links from pdf and replacing them with associated plain text - modular script but needs to be cleaned up

…ec.pdf

merging most recent upstream commits

updating the fork with most recent upstream commits

Apart from formatting and other minor changes, this commit includes an initial edit to the .circleci config file to try and build the pdf alongside circleci build of mkdocs

Final working changes from the following files were imported into this commit: - ./.circleci/config.yml - pdf_build_src/build_pdf.sh - src/04-modality-specific-files/01-magnetic-resonance-imaging-data.md

Arshitha · 2020-02-25T20:56:28Z

The rendered pdf can be found under the artifacts tab of the circleci build_docs_pdf test details.
For easy reference: bids-spec.pdf

sappelhoff · 2020-02-26T09:22:42Z

@effigies @yarikoptic @nicholst @franklin-feingold @robertoostenveld

I am pinging you because you have been involved in discussions about the PDF generation in the past (please help me to ping those people I forgot).

We have gone through several iterations with this PR now with @Arshitha busily fixing bugs and shaping the PR to our requests.

There is still a lot to be done (as outlined in the OP above by @Arshitha ) ... but I think we should merge this PR first, because it provides a solid foundation.

If you think so too, please review and approve ... disapprove only if you find issues that should definitely not make it into the bids-specification repo ... and then tell us about these issues so we can stay active :-)

PS: I tried rebasing this PR and it was hell (I failed for now) ... if somebody else wants to train some git kung fu, please try to rebase this PR and make a new PR that we can merge.

Else we can look into making a merge commit --> but I already see several things that'd need to be resolved (see https://github.com/bids-standard/bids-specification/pull/427/files#r384366609).

sappelhoff · 2020-02-26T09:24:40Z

.circleci/config.yml

@@ -57,7 +73,7 @@ jobs:
           working_directory: ~/build
           command: |
             if (git log -1 --pretty=%s | grep Merge*) && (! git log -1 --pretty=%b | grep REL:) ; then
-             github_changelog_generator --user bids-standard --project bids-specification --token ${CHANGE_TOKEN} --output ~/build/CHANGES.md --base ~/build/src/pregh-changes.md --header-label Changelog --no-issues --no-issues-wo-labels --no-filter-by-milestone --no-compare-link --pr-label ""
+             github_changelog_generator --user bids-standard --project bids-specification --token ${CHANGE_TOKEN} --output ~/build/CHANGES.md --base ~/build/src/pregh-changes.md --header-label "# Changelog" --no-issues --no-issues-wo-labels --no-filter-by-milestone --no-compare-link --pr-label ""


I am wondering why this line is highlighted as a diff.

In master, we should have the exact same line:

bids-specification/.circleci/config.yml

Line 60 in 6bd2ad5

github_changelog_generator --user bids-standard --project bids-specification --token ${CHANGE_TOKEN} --output ~/build/CHANGES.md --base ~/build/src/pregh-changes.md --header-label "# Changelog" --no-issues --no-issues-wo-labels --no-filter-by-milestone --no-compare-link --pr-label ""

... so there shouldn't be a change, actually 🤔

I am not sure why that's the case. I remember having synced my fork with the latest commits on master, but perhaps I missed something when cherry picking commits from here #400 ?

nicholst

I can't comment on the machinery of generating the PDF and integrating it with the repo/website, but I've reviewed the PDF and believe the remaining snags are minor enough and thus it is worthy of merging.

Arshitha · 2020-02-28T15:04:28Z

PS: I tried rebasing this PR and it was hell (I failed for now) ... if somebody else wants to train some git kung fu, please try to rebase this PR and make a new PR that we can merge.

Else we can look into making a merge commit --> but I already see several things that'd need to be resolved (see https://github.com/bids-standard/bids-specification/pull/427/files#r384366609).

@sappelhoff would it be easier if I were to create a new branch from master and then just add all of the new/changed files to this new branch to create a PR from there? That way, it won't have more than 2-3 commits and therefore, easier to rebase/merge?

This PR can then be deleted?

Arshitha · 2020-02-28T15:20:37Z

Also, most of the conflicts to be resolved, if this were to be merged, would be column width adjustments in the markdown files.

franklin-feingold

thank you @Arshitha for the time and effort put into generating a fantastic pdf of the specification! The pdf looks great!! can iterate on the remaining open issues
thank you @sappelhoff @nicholst @effigies for providing great guidance and shepherding

sappelhoff · 2020-03-01T08:56:00Z

@sappelhoff would it be easier if I were to create a new branch from master and then just add all of the new/changed files to this new branch to create a PR from there?

@Arshitha that's how I would do it ... :-) but we could perhaps also just make a merge commit without rebasing this PR. I am only puzzled by the "# Changelog" diff, which shouldn't be a diff 🤔

perhaps @effigies can comment, as he has more git experience.

Arshitha · 2020-03-09T19:46:36Z

@sappelhoff can we go ahead and merge by creating a new PR with only a couple of commits or do we still try rebasing or merging this one?

sappelhoff · 2020-03-10T08:45:55Z

@Arshitha I did not resolve the question with the unexpected diff that's showing up (https://github.com/bids-standard/bids-specification/pull/427/files#r384366609).

So yes, perhaps it'd be best if you could

get your fork up to date with current master
make a new branch
add the new contents of the PDF generation
make a new PR

I'll be sure to make a quick check then and merge. We have all the reviews (and approvals) we need :-)

Arshitha added 21 commits November 22, 2019 15:49

changes to generate pdf-version of the specs

b6d9875

included name to contributors file

30e3aab

Merge pull request #1 from bids-standard/master

877a303

merging changes from base repo

Merge branch 'pdf-version' into master

5a5dc3f

updated branch with most recent commits from base repo

4fefe5b

deleted all files

4a2601c

added most recent version of files

8aa83e6

new branch with more readable commit history

4794112

- automated header extraction from changelog - removing all internal links from pdf and replacing them with associated plain text - modular script but needs to be cleaned up

removed remove_cross_internal_links function

a5667bf

added comments and cleaned up code

c00d029

added readme and cleaned up code

a3efecf

reverted modifications to CHANGES.md and fixed file naming to bids-sp…

e9538f4

…ec.pdf

adding title and version details to the cover page

626f1a4

Merge remote-tracking branch 'upstream/master'

0ec12cb

merging most recent upstream commits

Merge branch 'master' into updated-pdf-version-specs

3476e74

updating the fork with most recent upstream commits

STY: Fixed Markdown Style

73caef4

Changes based on code review

07084c7

Apart from formatting and other minor changes, this commit includes an initial edit to the .circleci config file to try and build the pdf alongside circleci build of mkdocs

cleaned up code and final checks with circleci

980894f

resolving merge conflicts in circleci config file

5074814

importing changes from 'updated-pdf-version-specs' branch

fd71fb9

Final working changes from the following files were imported into this commit: - ./.circleci/config.yml - pdf_build_src/build_pdf.sh - src/04-modality-specific-files/01-magnetic-resonance-imaging-data.md

Arshitha requested review from chrisgorgo and sappelhoff as code owners February 25, 2020 20:29

Arshitha changed the title ~~Pdf version specs~~ pdf version specs Feb 25, 2020

Arshitha mentioned this pull request Feb 25, 2020

Updated pdf version specs #400

Closed

column width adjustment

e41e813

sappelhoff reviewed Feb 26, 2020

View reviewed changes

nicholst approved these changes Feb 26, 2020

View reviewed changes

franklin-feingold approved these changes Feb 28, 2020

View reviewed changes

Arshitha mentioned this pull request Mar 10, 2020

[INFRA] add support for building PDF versions of the spec #431

Merged

sappelhoff closed this Mar 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pdf version specs #427

pdf version specs #427

Arshitha commented Feb 25, 2020 •

edited

Loading

Arshitha commented Feb 25, 2020 •

edited

Loading

sappelhoff commented Feb 26, 2020 •

edited

Loading

sappelhoff Feb 26, 2020

Arshitha Feb 28, 2020

nicholst left a comment

Arshitha commented Feb 28, 2020

Arshitha commented Feb 28, 2020

franklin-feingold left a comment •

edited

Loading

sappelhoff commented Mar 1, 2020

Arshitha commented Mar 9, 2020

sappelhoff commented Mar 10, 2020

pdf version specs #427

pdf version specs #427

Conversation

Arshitha commented Feb 25, 2020 • edited Loading

Bugs Fixed:

Enhancements required (non-exhaustive list):

Arshitha commented Feb 25, 2020 • edited Loading

sappelhoff commented Feb 26, 2020 • edited Loading

sappelhoff Feb 26, 2020

Choose a reason for hiding this comment

Arshitha Feb 28, 2020

Choose a reason for hiding this comment

nicholst left a comment

Choose a reason for hiding this comment

Arshitha commented Feb 28, 2020

Arshitha commented Feb 28, 2020

franklin-feingold left a comment • edited Loading

Choose a reason for hiding this comment

sappelhoff commented Mar 1, 2020

Arshitha commented Mar 9, 2020

sappelhoff commented Mar 10, 2020

Arshitha commented Feb 25, 2020 •

edited

Loading

Arshitha commented Feb 25, 2020 •

edited

Loading

sappelhoff commented Feb 26, 2020 •

edited

Loading

franklin-feingold left a comment •

edited

Loading