search

hammerlab / guacamole

Spark-based variant calling, with experimental support for multi-sample somatic calling (including RNA) and local assembly
Apache License 2.0
84 stars 21 forks source link

space out lots of joint caller code blocks #541

Closed ryan-williams closed 8 years ago

ryan-williams commented 8 years ago

hopefully improving readability

This change is Reviewable

timodonnell commented 8 years ago

Made some comments. I am assuming none of this changes semantics. My main thought is cases where you have taken unnamed things like pair._1 and given them names, are a big improvement, but cases where all that changed was spacing are more debatable, especially things involving parens on their own line or blank lines. I find it pretty useful to be able to look at a function in one screen full. I also am less into scala's for than you so I'd encourage you to not that change things to for when it is avoidable. Aside from comments I'm fine to merge this in.

ryan-williams commented 8 years ago

Cool, I responded to all comments and changed a few things.

One thing I want to mention in response to:

I find it pretty useful to be able to look at a function in one screen full.

is that I frequently use IntelliJ's code-block-collapsing capability to be able to view a file at higher and lower levels. In particular, ⌘⇧- to collapse everything then ⌘+ to expand just one level at a time is a great way to see just the top-level API of a class, and incrementally drill down:

In the last frame here I'm able to survey an almost 300-line method easily.

This is a good way to view a function on one screen as you've described, without us being constrained to pack logical expressions way further horizontally than is optimal for close-inspection use cases.

timodonnell commented 8 years ago

Cool, thanks for the responses @ryan-williams . LGTM