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

Update the Deep Review with latest Manubot #681

Merged
merged 202 commits into from
Nov 3, 2017

Conversation

dhimmel
Copy link
Collaborator

@dhimmel dhimmel commented Nov 3, 2017

Closes #680

cgreene and others added 30 commits August 5, 2016 10:34
Add in stub files so that we can start writing.
* Enable tag citations

See greenelab#2 (comment).

* Reflow text to satisfy codeclimate
…and-scaling

Hardware Limitations and Scaling
@dhimmel
Copy link
Collaborator Author

dhimmel commented Nov 3, 2017

The original work for this PR begins in ea92847

You can browse what the repo will look like if this PR is merged at this tree.

Here's the PDF from a local build: manuscript.pdf.

  • Before merging remember to create output branch to replace references branch.

@agitter ready for review

@dhimmel dhimmel requested a review from agitter November 3, 2017 21:07
@agitter
Copy link
Collaborator

agitter commented Nov 3, 2017

@dhimmel I should have time to review this weekend

@dhimmel
Copy link
Collaborator Author

dhimmel commented Nov 3, 2017

I should have time to review this weekend

@agitter if okay with you, I recommend we merge now, since I anticipate people may begin making revisions this weekend and I don't want them to start off of the old repository.

I could then address any issues you identify in subsequent commits.

@agitter
Copy link
Collaborator

agitter commented Nov 3, 2017

@dhimmel I can go through quickly now

Copy link
Collaborator

@agitter agitter left a comment

Choose a reason for hiding this comment

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

I didn't inspect everything, but I didn't find any showstoppers in the parts I reviewed. There are two things I recommend changing now and others we can clean up post-merge. You can merge when you're ready.

Pre-merge suggestions:

  • Keep authors/author-order.ipynb in the repository, that is our public record of how the author ordering was done and may have been referenced externally
  • Fix the broken templating in the Author contributions section

Post-merge:

  • Inspect pdf formatting and references in detail
  • Customize the front matter and maybe the back matter. In the expanded format we lead with three pages of authors, which is too long
  • Add back footnotes for corresponding authors and about the author order
  • Clarify what is no longer templated (contributions and funding are not?)
  • Perhaps rename manuscript.pdf back to deep-review.pdf along with the other outputs
  • Add back the example tag in USAGE.md
  • Double check that all author info transferred correctly
  • Check the build warnings arXiv article 1702.05747 published in"None" at https://doi.org/10.1016/j.media.2017.07.005, etc.
  • Make a note to all authors about the changes such as the ; between multiple references and prohibited characters in citations

@dhimmel dhimmel merged commit 8eb858a into greenelab:master Nov 3, 2017
dhimmel added a commit that referenced this pull request Nov 3, 2017
This build is based on
8eb858a.

This commit was created by the following Travis CI build and job:
https://travis-ci.org/greenelab/deep-review/builds/297004423
https://travis-ci.org/greenelab/deep-review/jobs/297004424

[ci skip]

The full commit message that triggered this build is copied below:

Merge PR #681 to update Manubot

Update the Deep Review with latest Manubot
dhimmel added a commit that referenced this pull request Nov 3, 2017
This build is based on
8eb858a.

This commit was created by the following Travis CI build and job:
https://travis-ci.org/greenelab/deep-review/builds/297004423
https://travis-ci.org/greenelab/deep-review/jobs/297004424

[ci skip]

The full commit message that triggered this build is copied below:

Merge PR #681 to update Manubot

Update the Deep Review with latest Manubot
@dhimmel dhimmel deleted the great-merge branch November 6, 2017 19:05
@agitter
Copy link
Collaborator

agitter commented Nov 9, 2017

@dhimmel should I create a new issue to track the items above that need to be fixed or double-checked?

@dhimmel
Copy link
Collaborator Author

dhimmel commented Nov 9, 2017

should I create a new issue to track the items above that need to be fixed or double-checked?

Yep, and you can assign me the ones you want me to do!

Some quick notes:

Add back the example tag in USAGE.md

This seems like it should be an upstream manubot-rootstock change?

Perhaps rename manuscript.pdf back to deep-review.pdf along with the other outputs

We could create a symlink pointing to manuscript.pdf. I'd only do this if we think there are public links (outside of this repo) to deep-review.pdf.

The arXiv build warnings are because those studies have now been published with DOIs. We can fix these at the end or sooner?

Make a note to all authors about the changes such as the ; between multiple references and prohibited characters in citations

This should be partly done in the updated README.

@agitter
Copy link
Collaborator

agitter commented Nov 9, 2017

Add back the example tag in USAGE.md

We had a Deep Review-specific example, [@tag:Zhou2015_deep_sea], that I had in mind. Any example from citation-tags.tsv would be fine, in which case it wouldn't be in the upstream manubot-rootstock.

Perhaps rename manuscript.pdf back to deep-review.pdf along with the other outputs

We can leave this alone. It was only my very minor personal preference.

The arXiv build warnings are because those studies have now been published with DOIs.

We could fix them now or immediately before resubmitting. I expect other preprints could be published. I just don't want to forget to do it.

This should be partly done in the updated README.

Your README update took care of the semi-colon. I'll use my #696 for the forbidden tags.

This was referenced Nov 10, 2017
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.

9 participants