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

add style guide for IDE's #1386

Merged
merged 1 commit into from
Jun 21, 2019
Merged

Conversation

mitochon
Copy link
Contributor

@mitochon mitochon commented Jun 14, 2019

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:

  • Add a non-binding style guide and people can adopt it if they like.
  • This is stolen from Google's styleguide with one mofication => change indent size from 2 to 4 which seem to be the default here.

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

  • [n/a ] Code compiles correctly
  • [ n/a] New tests covering changes and new functionality
  • [ n/a] All tests passing
  • Extended the README / documentation, if necessary <- let me know if any is needed
  • [ n/a] Is not backward compatible (breaks binary or source compatibility)

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
Copy link

codecov-io commented Jun 14, 2019

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
Copy link
Contributor

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

@mitochon
Copy link
Contributor Author

@yfarjoun updated to remove eclipse formatter

@lindenb
Copy link
Contributor

lindenb commented Jun 18, 2019

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

@mitochon
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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

Copy link
Member

@lbergelson lbergelson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mitochon I've been resistant to requiring an auto linted style guide, but I think committing a suggested one that generally matches how we format things is a good idea. 👍

@lbergelson lbergelson merged commit f68108d into samtools:master Jun 21, 2019
@mitochon mitochon deleted the 1196-add-formatter branch February 16, 2024 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants