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

use testFile in some tests #1268

Merged
merged 2 commits into from Nov 15, 2016
Merged

use testFile in some tests #1268

merged 2 commits into from Nov 15, 2016

Conversation

@ryan-williams
Copy link
Member

ryan-williams commented Nov 15, 2016

No description provided.

Copy link
Member

heuermh left a comment

Thanks for the cleanup! LGTM

@AmplabJenkins
Copy link

AmplabJenkins commented Nov 15, 2016

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

Copy link
Member

fnothaft left a comment

One small nit. Otherwise LGTM!

import java.io.File
import org.apache.spark.rdd.RDD
import org.bdgenomics.adam.rdd.ADAMContext._

This comment has been minimized.

Copy link
@fnothaft

fnothaft Nov 15, 2016

Member

Can you clean up the newline here?

This comment has been minimized.

Copy link
@ryan-williams

ryan-williams Nov 15, 2016

Author Member

mm my IntelliJ's ⌘⇧O defaults to a blank line between java imports, others, and scala imports:

I feel like that's a style I've seen around out in the world. I guess ADAM is not set up that way, but it also seems like people are not using IntelliJ's (or anything else's) import-optimization helpers in the codebase, as I've seen multiple unused imports laying around.

Also I'm not sure if I can change that IntelliJ setting per-project :( but lmk if given all this you still want to just have "one import blob" be the rule.

This comment has been minimized.

Copy link
@heuermh

heuermh Nov 15, 2016

Member

I admit to opening up IntelliJ only rarely, and am likely the culprit for some of those unused imports. If you have settings that work well with our style, maybe we could add them to the contributing doc.

This comment has been minimized.

Copy link
@fnothaft

fnothaft Nov 15, 2016

Member

Also I'm not sure if I can change that IntelliJ setting per-project :( but lmk if given all this you still want to just have "one import blob" be the rule.

That would be my preference for consistency with the remainder of the codebase. If it's a big hassle from your side, I can fix on merge.

This comment has been minimized.

Copy link
@ryan-williams

ryan-williams Nov 15, 2016

Author Member

@fnothaft I changed it

Intersting @heuermh, what do you edit in?

import org.apache.spark.rdd.RDD
import org.bdgenomics.adam.rdd.ADAMContext._

import com.google.common.io.Files

This comment has been minimized.

Copy link
@fnothaft

fnothaft Nov 15, 2016

Member

Are we still importing Files here?

This comment has been minimized.

Copy link
@ryan-williams

ryan-williams Nov 15, 2016

Author Member

not sure I understand the q; Files is still used in this file.

This comment has been minimized.

Copy link
@fnothaft

fnothaft Nov 15, 2016

Member

Yeah, I just wanted to confirm that.

@fnothaft
Copy link
Member

fnothaft commented Nov 15, 2016

I can't speak for @heuermh but I never use Intellij either and do all my editing in emacs. I tried switching to Intellij several years back but it's emacs compat mode is pretty rough. I code in a variety of other languages, so cutting over and switching to Intellij doesn't make sense as an investment for me.

@fnothaft
Copy link
Member

fnothaft commented Nov 15, 2016

Will merge on tests rerunning and passing.

@heuermh
Copy link
Member

heuermh commented Nov 15, 2016

Emacs and whatever scala mode comes in via melpa for me. I try to look at all my prs in IntelliJ for the static code checking at least once, but the way it handles git makes me nervous, so I don't use it full time. And I prefer running in terminals. :)

@AmplabJenkins
Copy link

AmplabJenkins commented Nov 15, 2016

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

@fnothaft
Copy link
Member

fnothaft commented Nov 15, 2016

Jenkins, retest this please.

@ryan-williams
Copy link
Member Author

ryan-williams commented Nov 15, 2016

why retest OOC?

@fnothaft
Copy link
Member

fnothaft commented Nov 15, 2016

@ryan-williams I just merged #1135 which was +~2500 -~600 so I am somewhat distrustful of old Jenkins results is all.

@AmplabJenkins
Copy link

AmplabJenkins commented Nov 15, 2016

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

@fnothaft fnothaft merged commit 1d2132c into bigdatagenomics:master Nov 15, 2016
1 check passed
1 check passed
default Merged build finished.
Details
@fnothaft
Copy link
Member

fnothaft commented Nov 15, 2016

Merged! Thanks @ryan-williams!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

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