Cleaning up formatting and spacing of docs. #1640

devin-petersohn · 2017-07-26T21:49:22Z

Resolves #1638.

Wrapped lines at 80 if possible, also addressed formatting and spacing issues from rendering of the doc.

coveralls · 2017-07-26T22:04:16Z

Coverage remained the same at 83.961% when pulling 76913b9 on devin-petersohn:issue#1638 into c8a2202 on bigdatagenomics:master.

AmplabJenkins · 2017-07-26T22:14:23Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/2285/
Test PASSed.

fnothaft

Few small nits. Thanks @devin-petersohn!

fnothaft · 2017-07-26T22:15:15Z

docs/source/60_building_apps.md

-.scala source file for each CLI action (e.g. [Transform.scala](https://github.com/bigdatagenomics/adam/blob/master/adam-cli/src/main/scala/org/bdgenomics/adam/cli/Transform.scala)
-for the [transform](#transform) action), and a main class ([ADAMMain.scala](https://github.com/bigdatagenomics/adam/blob/master/adam-cli/src/main/scala/org/bdgenomics/adam/cli/ADAMMain.scala))
+.scala source file for each CLI action (e.g.
+[Transform.scala](https://github.com/bigdatagenomics/adam/blob/master/adam-cli/src/main/scala/org/bdgenomics/adam/cli/Transform.scala)


Update to TransformAlignments.

fnothaft · 2017-07-26T22:54:06Z

docs/source/60_building_apps.md

+The `MyCommandArgs` class defined above is provided in the constructor and
+specifies the generic type for `BDGSparkCommand`. The companion object defined
+above is declared as a field.  For access to an [slf4j](http://www.slf4j.org/)
+Logger via the `log` field, specify `with Logging`.


Suggest:

"...log field, mix in the org.bdgenomics.utils.misc.Logging trait by adding with Logging to the class definition."

fnothaft · 2017-07-26T22:54:58Z

docs/source/60_building_apps.md

-[slf4j](http://www.slf4j.org/) Logger via the `log` field, specify `with Logging`.
+Extend `BDGSparkCommand` class and implement the `run(SparkContext)` method.
+The `MyCommandArgs` class defined above is provided in the constructor and
+specifies the generic type for `BDGSparkCommand`. The companion object defined


"specifies the generic type" is weird phrasing.

fnothaft · 2017-07-26T22:55:53Z

docs/source/60_building_apps.md

+Extend `BDGSparkCommand` class and implement the `run(SparkContext)` method.
+The `MyCommandArgs` class defined above is provided in the constructor and
+specifies the generic type for `BDGSparkCommand`. The companion object defined
+above is declared as a field.  For access to an [slf4j](http://www.slf4j.org/)


Can you add docs explaining why we define the companion object? In general, prefer "why" to "what".

fnothaft · 2017-07-26T22:56:22Z

docs/source/60_building_apps.md

@@ -77,7 +83,7 @@ Add the new command to the default list of commands in `ADAMMain`.

 Build ADAM and run the new command via `adam-submit`.

-```bash
+```


Please revert deletion of bash.

I deleted the bash specification here because it caused non-bash text to render with text highlighting. Removing bash does not affect anything except the incorrectly highlighted text. In the rendered version of this PR, you can see an example of both bash tagged and not.

Ah, I see. Thanks for the clarification; I thought it was an incidental deletion, but you are right, this should not be bash formatted.

fnothaft · 2017-07-26T22:57:12Z

docs/source/60_building_apps.md

@@ -103,15 +109,16 @@ ADAM ACTIONS
 $ ./bin/adam-submit myCommand input.foo
 ```

-Then consider making a pull request to include the new command in ADAM!
+Then consider making a
+[pull request](https://github.com/bigdatagenomics/adam/pulls) to include the


Correct link for opening a PR would be https://github.com/bigdatagenomics/adam/compare?expand=1

As an aside, I might add docs asking people to give us a heads up by opening an issue prior to opening a PR.

So add a section somewhere about contributing to the repo? Should this be a new doc, or does it belong in the into section?

We have a detailed CONTRIBUTING.md with info for people contributing. Just add a heads up that they should float the issue first, because the way we've written it here makes it sound like we'll merge any and all PRs against ADAM. We've tried to bias towards accepting contributions, but we can't accept everything, and we're probably more hesitant to accept CLI additions relative to API additions.

fnothaft · 2017-07-26T22:59:14Z

docs/source/60_building_apps.md

-instead of editing `ADAMMain` to add new commands as above, create a new
-object with a `main(args: Array[String])` method that delegates to `ADAMMain`
-and provides additional command(s) via its constructor.
+To extend the ADAM CLI by adding new commands in an *external* repository,


Please drop italics; in general, avoid italics

fnothaft · 2017-07-26T23:00:29Z

docs/source/60_building_apps.md

-application to improve performance.
+Create an Apache Spark configuration `SparkConf` and use it to create a new
+`SparkContext`. The following serialization configuration needs to be present
+o register ADAM classes. If any additional


o -> to

coveralls · 2017-07-31T16:28:15Z

Coverage remained the same at 83.961% when pulling 1bf855b on devin-petersohn:issue#1638 into c8a2202 on bigdatagenomics:master.

fnothaft

Couple of small last fixes we should get in. Thanks @devin-petersohn!

fnothaft · 2017-08-02T16:13:28Z

docs/source/60_building_apps.md

-ADAM is packaged so that it can be used interatively via the ADAM shell, called from
-the command line interface (CLI), or included as a library when building downstream
-applications.
+ADAM is packaged so that it can be used interatively via the ADAM shell, called


interatively -> interactively

fnothaft · 2017-08-02T16:17:20Z

docs/source/60_building_apps.md

-for the [transform](#transform) action), and a main class ([ADAMMain.scala](https://github.com/bigdatagenomics/adam/blob/master/adam-cli/src/main/scala/org/bdgenomics/adam/cli/ADAMMain.scala))
+.scala source file for each CLI action (e.g.
+[TransformAlignments.scala](https://github.com/bigdatagenomics/adam/blob/master/adam-cli/src/main/scala/org/bdgenomics/adam/cli/TransformAlignments.scala)
+for the [transform](#transform) action), and a main class


transform -> transformAlignments

fnothaft · 2017-08-02T16:18:35Z

docs/source/60_building_apps.md

-Extend `Args4jBase` class to specify arguments to the command. Arguments are defined using
-the [args4j library](http://args4j.kohsuke.org/). If reading from or writing to Parquet,
-consider including Parquet arguments via `with ParquetArgs`.
+Extend `Args4jBase` class to specify arguments to the command. Arguments are


While we're updating this section, thoughts on making this a numbered list?

Alternatively, numbered list under To add a new command: and add subsection headings? e.g.,

To add a new command: 1. [Extend Args4jBase to specify arguments](#extend-arguments) 2. [Extend BDGCommandCompanion](#extend-companion) ... #### Extend Args4jBase to specify arguments {#extend-arguments}

I think the second strategy would be a bit easier to follow.

fnothaft · 2017-08-02T16:26:46Z

docs/source/60_building_apps.md

+to register ADAM classes. If any additional
+[Kyro serializers](https://github.com/EsotericSoftware/kryo) need to be
+registered,
+[create a registrator that delegates to the ADAM registrator](#registrator).


From a word wrap perspective, you shouldn't need the whole link text to be on a single line, no?

AmplabJenkins · 2017-08-03T04:03:01Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/2312/
Test FAILed.

devin-petersohn · 2017-08-03T04:06:16Z

docs/source/60_building_apps.md

-Extend `Args4jBase` class to specify arguments to the command. Arguments are defined using
-the [args4j library](http://args4j.kohsuke.org/). If reading from or writing to Parquet,
-consider including Parquet arguments via `with ParquetArgs`.
+1. [Extend Args4jBase to specify arguments](#extend-arguments)


How does this look @fnothaft?

I think this looks great, thanks @devin-petersohn!

devin-petersohn · 2017-08-03T04:07:53Z

docs/source/60_building_apps.md

 * Use ADAM as a [library in new applications](#library)


 ## Extend the ADAM CLI by adding new commands {#commands}

 ADAM's CLI is implemented in the adam-cli Apache Maven module of the
 [bdgenomics/adam](https://github.com/bigdatagenomics/adam) repository, one
-.scala source file for each CLI action (e.g. [Transform.scala](https://github.com/bigdatagenomics/adam/blob/master/adam-cli/src/main/scala/org/bdgenomics/adam/cli/Transform.scala)
-for the [transform](#transform) action), and a main class ([ADAMMain.scala](https://github.com/bigdatagenomics/adam/blob/master/adam-cli/src/main/scala/org/bdgenomics/adam/cli/ADAMMain.scala))
+.scala source file for each CLI action (e.g.


I thought about rephrasing this a little bit because the flow gets broken by each example. Thoughts?

I think this is fine as is, but if you'd like to make a pass at touching it up, that's a-OK with me.

Let's go with the "if it's not broken, don't fix it" principle here. We can leave as-is.

devin-petersohn · 2017-08-03T04:11:34Z

Thanks for your patience. I have a couple of comments I'd like feedback on.

fnothaft · 2017-08-03T05:22:10Z

Merged! Thanks @devin-petersohn!

Cleaning up formatting and spacing of docs.

76913b9

fnothaft requested changes Jul 26, 2017

View reviewed changes

devin-petersohn added 2 commits July 31, 2017 09:04

Addressing reviewer comments

a6fb4f8

Addressing reviewer comments

1bf855b

fnothaft requested changes Aug 2, 2017

View reviewed changes

Addressing reviewer comments.

2ab2f3a

devin-petersohn commented Aug 3, 2017

View reviewed changes

fnothaft approved these changes Aug 3, 2017

View reviewed changes

fnothaft merged commit 96fc37f into bigdatagenomics:master Aug 3, 2017

heuermh added this to the 0.23.0 milestone Dec 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleaning up formatting and spacing of docs. #1640

Cleaning up formatting and spacing of docs. #1640

devin-petersohn commented Jul 26, 2017 •

edited

Loading

coveralls commented Jul 26, 2017 •

edited

Loading

AmplabJenkins commented Jul 26, 2017

fnothaft left a comment

fnothaft Jul 26, 2017

fnothaft Jul 26, 2017

fnothaft Jul 26, 2017

fnothaft Jul 26, 2017

fnothaft Jul 26, 2017

devin-petersohn Jul 26, 2017

fnothaft Jul 26, 2017

fnothaft Jul 26, 2017

fnothaft Jul 26, 2017

devin-petersohn Jul 26, 2017

fnothaft Jul 26, 2017 •

edited

Loading

fnothaft Jul 26, 2017

fnothaft Jul 26, 2017

coveralls commented Jul 31, 2017 •

edited

Loading

fnothaft left a comment

fnothaft Aug 2, 2017

fnothaft Aug 2, 2017

fnothaft Aug 2, 2017

fnothaft Aug 2, 2017

fnothaft Aug 2, 2017

fnothaft Aug 2, 2017

AmplabJenkins commented Aug 3, 2017

devin-petersohn Aug 3, 2017

fnothaft Aug 3, 2017

devin-petersohn Aug 3, 2017

fnothaft Aug 3, 2017

devin-petersohn Aug 3, 2017

fnothaft Aug 3, 2017

devin-petersohn commented Aug 3, 2017

fnothaft commented Aug 3, 2017

Cleaning up formatting and spacing of docs. #1640

Cleaning up formatting and spacing of docs. #1640

Conversation

devin-petersohn commented Jul 26, 2017 • edited Loading

coveralls commented Jul 26, 2017 • edited Loading

AmplabJenkins commented Jul 26, 2017

fnothaft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fnothaft Jul 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 31, 2017 • edited Loading

fnothaft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmplabJenkins commented Aug 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devin-petersohn commented Aug 3, 2017

fnothaft commented Aug 3, 2017

devin-petersohn commented Jul 26, 2017 •

edited

Loading

coveralls commented Jul 26, 2017 •

edited

Loading

fnothaft Jul 26, 2017 •

edited

Loading

coveralls commented Jul 31, 2017 •

edited

Loading