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

Upgrading repo to track manubot-rootstock #680

Closed
dhimmel opened this issue Nov 3, 2017 · 6 comments
Closed

Upgrading repo to track manubot-rootstock #680

dhimmel opened this issue Nov 3, 2017 · 6 comments

Comments

@dhimmel
Copy link
Collaborator

dhimmel commented Nov 3, 2017

The Deep Review predates the Manubot (the CI-based system we use to process the manuscript). In fact, greenelab/manubot-rootstock (a template GitHub repo for Manubot instances) and greenelab/manubot (a python package for processing the manuscript) were both derived from the code from Deep Review. In other words, Deep Review gave birth to these projects.

However, the Manubot has now diverged considerably from the codebase and architecture present in this repository. The manuscript processing code in this repository has not been receiving any rootstock updates. As time goes on, the reconciliation of the codebases becomes more difficult. However, once synchronized, incremental updates should generally be straightforward.

In #678 (comment), I proposed merging in the latest manubot-rootstock. @agitter replied:

@dhimmel maybe we should open a new issue to preview and discuss how much would be involved in the merge. I'm hesitant to create a lot of new work when the current version of deep review is running smoothly even if it lacks some (important) features.

The merge certainly could be difficult. I'm happy to take on the task with a focus on minimizing disruption to open pull requests. The main benefits I see to the upgrade are:

  • getting new Manubot features including improved CI build feedback, formatting, and architecture
  • providing users with an up-to-date Manubot experience. The Deep Review is the Manubot instance with the greatest number of contributors. So we should strive to give users the cutting edge Manubot experience.
  • this repository has some customizations such as a non-standard (in terms of Manubot) authorship list. I'm interesting in seeing how the current Manubot architecture accommodates these needs. In other words, the Deep Review can provide important feedback for how to improve Manubot... but only if it's actually running Manubot.

Anyways, I'll attempt a merge and PR, so we can see exactly what the upgrade will entail. Most likely we will have to remove the current author scripts and re-engineer them, which I am willing to do.

@agitter
Copy link
Collaborator

agitter commented Nov 3, 2017

Good luck with the merge. This will also add proper figure and table support, which we'll need for the revisions.

How much has changed from the author perspective? The citation syntax is slightly different now. Is that it? We already asked all of the contributors learn one custom writing platform so I want to be conscious of how many new requirements we impose.

@dhimmel
Copy link
Collaborator Author

dhimmel commented Nov 3, 2017

How much has changed from the author perspective?

12ce17c contains the changes to the manuscript body. Really the only change that users will need to be aware of is to semicolon separate references. Once we see the effect on open PRs, I can post a message on every open PR regarding if any upgrade steps are needed and offer help if neccessary.

@dhimmel
Copy link
Collaborator Author

dhimmel commented Nov 3, 2017

I'd also like to start phasing in "one line per sentence" rather than word wrap. I think the best way to do this would be to operate on sections without any open PRs. And then when editing sections, we can encourage one line per sentence. However, this would be in future PRs not #681

@agitter
Copy link
Collaborator

agitter commented Nov 3, 2017

I'd also like to start phasing in "one line per sentence" rather than word wrap

We had an issue about that and there was broad consensus that we should convert. Someone had a simple script to implement the change. I don't have the issue number accessible right now though.

@dhimmel
Copy link
Collaborator Author

dhimmel commented Nov 3, 2017

We had an issue about that

#624 proposed by @michaelmhoffman. @michaelmhoffman if you are interested in doing this, then you can take the lead (wait for #681 to be merged). Otherwise I can.

@michaelmhoffman
Copy link
Contributor

I would like to punt to @evancofer who already wrote a script to do this.

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

No branches or pull requests

3 participants