samtools / htsjdk

A Java API for high-throughput sequencing data (HTS) formats.
http://samtools.github.io/htsjdk/
283 stars 242 forks source link

add style guide for IDE's #1386

Closed mitochon closed 5 years ago

mitochon commented 5 years ago

Description

I recently submitted my first PR to this project. I find that the code style is heterogenous which sometimes distracts me from working.

I am used to having an IDE format things but I realize this may not work for everybody. I see some discussion already in #1196 about enforcing certain styles.

My proposal is the following:

I'm relying on inertia, with the assumption that people won't reformat files that's already reformatted, and new changes will mostly be in-line with the proposed format.

Hopefully in the long run the style will converge, but initially there may be a lot of PR's with a lot of formatting changes when people start using their IDE to reformat stuff.

Side note: I asked about this earlier in the gitter channel and @cmnbroad suggested to try (see logs on May 21 and May 22)

Checklist

References

Diff between proposed version and Google's version (TLDR; increase indent from 2 -> 4)

eclipse formatter

3c3
< <profile kind="CodeFormatterProfile" name="htsjdkStyle" version="1">
---
> <profile kind="CodeFormatterProfile" name="GoogleStyle" version="13">
170c170
< <setting id="org.eclipse.jdt.core.formatter.tabulation.size" value="4"/>
---
> <setting id="org.eclipse.jdt.core.formatter.tabulation.size" value="2"/>

intellij formatter

< <code_scheme name="htsjdkStyle">
---
> <code_scheme name="GoogleStyle">
5c5
<       <option name="INDENT_SIZE" value="4" />
---
>       <option name="INDENT_SIZE" value="2" />
codecov-io commented 5 years ago

Codecov Report

Merging #1386 into master will not change coverage. The diff coverage is n/a.

@@             Coverage Diff             @@
##              master     #1386   +/-   ##
===========================================
  Coverage     68.007%   68.007%           
  Complexity      8352      8352           
===========================================
  Files            570       570           
  Lines          33848     33848           
  Branches        5661      5661           
===========================================
  Hits           23019     23019           
  Misses          8645      8645           
  Partials        2184      2184
yfarjoun commented 5 years ago

Given that we are not aware of maintainers who use eclipse, perhaps we only need the IntelliJ version?

mitochon commented 5 years ago

@yfarjoun updated to remove eclipse formatter

lindenb commented 5 years ago

@lindenb I am too late for this ? I use eclipse : I think it would be good for people writing PR

mitochon commented 5 years ago

Someone can make a call 😃

I can undo the last commit and put back the eclipse style guide. My personal opinion is its better to support more IDE options, as long as they are compatible with each other.

yfarjoun commented 5 years ago

as long as everything is compatible with current guidelines, and we have active folks that use the style_guide, I think it would be good to have them....thanks @lindenb for chiming in!

mitochon commented 5 years ago

All right, so I'll put the Eclipse style formatter back in per the above comment.