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 documentation & revision guidance #693

Merged
merged 13 commits into from
Nov 6, 2017

Conversation

dhimmel
Copy link
Collaborator

@dhimmel dhimmel commented Nov 6, 2017

Refs #678

README.md Outdated
pull request, and pitch in. Authorship criteria remain the same.
We are currently working on manuscript revisions in response to two external peer reviews.
See [Issue #678](https://github.com/greenelab/deep-review/issues/678) to coordinate this process.
When addressing reviewer comments, please also update [`response-to-reviewers.md`](content/response-to-reviewers.md) as appropriate.
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to make this stronger? We'd like to not accept PRs on new topics unless they directly address review comments? Basically just "may" -> "will"

Copy link
Member

@cgreene cgreene left a comment

Choose a reason for hiding this comment

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

couple suggested changes - largely LGTM, so just a few points


> The authors summarized over 400 literature references purely by narratives. The authors provided synopsis for each important reference, but lacks synthesis of related work. It would be better to synthesize related work into a table and analyze their characteristics.

TODO
Copy link
Member

Choose a reason for hiding this comment

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

For the ones that just say todo, do you want to add a link to the connected issue?

@dhimmel
Copy link
Collaborator Author

dhimmel commented Nov 6, 2017

Code Climate

@agitter and @cgreene have you found that code climate is doing anything useful for this repo? Is this something we should keep enabled but change the line length setting or disable entirely?

@cgreene
Copy link
Member

cgreene commented Nov 6, 2017

@cgreene
Copy link
Member

cgreene commented Nov 6, 2017

@dhimmel : now that we have travis, i am fine with disabling codeclimate - @agitter, do you agree?

@cgreene
Copy link
Member

cgreene commented Nov 6, 2017

LGTM 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.

Looks good. Feel free to disable code climate if you'd like. I rely on Travis testing more.

README.md Outdated

#### More about the manuscript.
**Manubot updates:**
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we also remind contributors to pull the latest version of master before making revisions? This was always the recommendation but will be even more important after #681.

README.md Outdated
We are currently working on manuscript revisions in response to two external peer reviews.
See [Issue #678](https://github.com/greenelab/deep-review/issues/678) to coordinate this process.
When addressing reviewer comments, please also update [`response-to-reviewers.md`](content/response-to-reviewers.md) as appropriate.
At the moment, efforts are concentrated on revisions: content unrelated to revisions may be deferred or not accepted.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I also would like to focus on merging existing pull requests, but this text is okay.

@@ -1,20 +1,11 @@
## Abstract {.page_break_before}

Deep learning, which describes a class of machine learning algorithms, has
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we try to merge some of the outstanding pull requests first? That may take too long, so I'm okay updating the line wrapping now if you think that is most efficient. We will need to provide help with merge conflicts for those pull requests though.

Copy link
Collaborator Author

@dhimmel dhimmel Nov 6, 2017

Choose a reason for hiding this comment

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

I only reflowed the abstract assuming that none of the PRs touch the abstract. Yes, we should focus on reviewing existing PRs. They likely all have conflicts now. Tag me when it's time to merge and I will handle the conflict resolution. Sooner the better.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@dhimmel ah, I see. I reviewed this hastily between meetings.

@@ -0,0 +1,48 @@
# Response to Reviewers
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is great 👍

@dhimmel dhimmel merged commit f12c1d7 into greenelab:master Nov 6, 2017
@dhimmel dhimmel deleted the revision-status branch November 6, 2017 19:46
dhimmel added a commit that referenced this pull request Nov 6, 2017
This build is based on
f12c1d7.

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

[ci skip]

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

Update documentation & revision guidance (#693)

* Add response to reviewers template
Refs #678

* Update CONTRIBUTING.md
semantic linefeeds
GitHub PR review interface

* Update README for current project status

* Link issues in response to reviewers

* Review formatting

* Code Climate: disable line_length for markdown

* Remove trailing whitespace

* Reflow abstract: one line per sentence

Refs #624

* Reviews: one line per sentence

This will help if quoted paragraphs are split when responding.

* Fix Blank line inside blockquote

* Concentration on reviews wording

* Prioritize open PR review

* README: keep forks synced
dhimmel added a commit that referenced this pull request Nov 6, 2017
This build is based on
f12c1d7.

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

[ci skip]

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

Update documentation & revision guidance (#693)

* Add response to reviewers template
Refs #678

* Update CONTRIBUTING.md
semantic linefeeds
GitHub PR review interface

* Update README for current project status

* Link issues in response to reviewers

* Review formatting

* Code Climate: disable line_length for markdown

* Remove trailing whitespace

* Reflow abstract: one line per sentence

Refs #624

* Reviews: one line per sentence

This will help if quoted paragraphs are split when responding.

* Fix Blank line inside blockquote

* Concentration on reviews wording

* Prioritize open PR review

* README: keep forks synced
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.

3 participants