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

Cleaning up formatting and spacing of docs. #1640

Merged
merged 4 commits into from Aug 3, 2017

Conversation

@devin-petersohn
Copy link
Member

@devin-petersohn devin-petersohn commented Jul 26, 2017

Resolves #1638.

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

@coveralls
Copy link

@coveralls coveralls commented Jul 26, 2017

Coverage Status

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

@AmplabJenkins
Copy link

@AmplabJenkins AmplabJenkins commented Jul 26, 2017

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.

Copy link
Member

@fnothaft fnothaft left a comment

Few small nits. Thanks @devin-petersohn!

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

This comment has been minimized.

@fnothaft

fnothaft Jul 26, 2017
Member

Update to TransformAlignments.

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

This comment has been minimized.

@fnothaft

fnothaft Jul 26, 2017
Member

Suggest:

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

[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

This comment has been minimized.

@fnothaft

fnothaft Jul 26, 2017
Member

"specifies the generic type" is weird phrasing.

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/)

This comment has been minimized.

@fnothaft

fnothaft Jul 26, 2017
Member

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

@@ -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
```

This comment has been minimized.

@fnothaft

fnothaft Jul 26, 2017
Member

Please revert deletion of bash.

This comment has been minimized.

@devin-petersohn

devin-petersohn Jul 26, 2017
Author Member

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.

This comment has been minimized.

@fnothaft

fnothaft Jul 26, 2017
Member

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

@@ -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

This comment has been minimized.

@fnothaft

fnothaft Jul 26, 2017
Member

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

This comment has been minimized.

@fnothaft

fnothaft Jul 26, 2017
Member

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

This comment has been minimized.

@devin-petersohn

devin-petersohn Jul 26, 2017
Author Member

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

This comment has been minimized.

@fnothaft

fnothaft Jul 26, 2017
Member

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.

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,

This comment has been minimized.

@fnothaft

fnothaft Jul 26, 2017
Member

Please drop italics; in general, avoid italics

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

This comment has been minimized.

@fnothaft

fnothaft Jul 26, 2017
Member

o -> to

@coveralls
Copy link

@coveralls coveralls commented Jul 31, 2017

Coverage Status

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

Copy link
Member

@fnothaft fnothaft left a comment

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

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

This comment has been minimized.

@fnothaft

fnothaft Aug 2, 2017
Member

interatively -> interactively

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

This comment has been minimized.

@fnothaft

fnothaft Aug 2, 2017
Member

transform -> transformAlignments

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

This comment has been minimized.

@fnothaft

fnothaft Aug 2, 2017
Member

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

This comment has been minimized.

@fnothaft

fnothaft Aug 2, 2017
Member

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}

This comment has been minimized.

@fnothaft

fnothaft Aug 2, 2017
Member

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

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

This comment has been minimized.

@fnothaft

fnothaft Aug 2, 2017
Member

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

@AmplabJenkins
Copy link

@AmplabJenkins AmplabJenkins commented Aug 3, 2017

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.

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)

This comment has been minimized.

@devin-petersohn

devin-petersohn Aug 3, 2017
Author Member

How does this look @fnothaft?

This comment has been minimized.

@fnothaft

fnothaft Aug 3, 2017
Member

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

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

This comment has been minimized.

@devin-petersohn

devin-petersohn Aug 3, 2017
Author Member

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

This comment has been minimized.

@fnothaft

fnothaft Aug 3, 2017
Member

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.

This comment has been minimized.

@devin-petersohn

devin-petersohn Aug 3, 2017
Author Member

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

This comment has been minimized.

@fnothaft

fnothaft Aug 3, 2017
Member

SGTM!

@devin-petersohn
Copy link
Member Author

@devin-petersohn devin-petersohn commented Aug 3, 2017

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

@fnothaft fnothaft merged commit 96fc37f into bigdatagenomics:master Aug 3, 2017
1 of 2 checks passed
1 of 2 checks passed
default Merged build finished.
Details
codacy/pr Good work! A positive pull request.
Details
@fnothaft
Copy link
Member

@fnothaft fnothaft commented Aug 3, 2017

Merged! Thanks @devin-petersohn!

@heuermh heuermh added this to the 0.23.0 milestone Dec 7, 2017
@heuermh heuermh added this to Completed in Release 0.23.0 Jan 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.