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

Style: no empty newlines at start or end of functions #15418

Merged
merged 13 commits into from
Apr 17, 2016
Merged

Style: no empty newlines at start or end of functions #15418

merged 13 commits into from
Apr 17, 2016

Conversation

tkelman
Copy link
Contributor

@tkelman tkelman commented Mar 9, 2016

These were the few outliers style-wise that looked like this. Mostly from @carlobaldassi.

@amitmurthy
Copy link
Contributor

@nalimilan
Copy link
Member

+1, we need to make the style guide more detailed.

@tkelman
Copy link
Contributor Author

tkelman commented Mar 10, 2016

Sure, if there's agreement on this point then it's just a matter of how to phrase the guideline.

@amitmurthy
Copy link
Contributor

Since source files are being changed to follow this style, may as well make it a guideline. I am fine with it.

@nalimilan
Copy link
Member

See #15439.

@andreasnoack
Copy link
Member

I think it can be useful for readability to have a newline in the beginning of a function when the signature is long. Is there a problem with allowing this? E.g. in terms of readability of diffs or something similar? Or is it mainly a matter of taste?

@tkelman
Copy link
Contributor Author

tkelman commented Mar 10, 2016

Consistency. And it wastes space in a way that doesn't clarify anything in my opinion.

@tbreloff
Copy link

Personally, I like a space when the function can be broken up cleanly, or if there's a comment on the first line:

function nospace()
  dosomethingshort()
  dosomethingshort()
end

function yesspace()

  # first do this
  this()

  # then that
  that()

  # finally other
  other()

end

@tkelman
Copy link
Contributor Author

tkelman commented Mar 11, 2016

This isn't unique to function definitions, any block opener/closer where you would typically indent the contents, having an empty newline right after the block opener or right before the end is redundant with the indentation and visually decouples the start and end of the block from its contents. It sticks out and doesn't look right to me. I'm all for whitespace between multiple steps, but before-start or after-finished don't count - that's what the function and the end are for.

@amitmurthy
Copy link
Contributor

I concur with Tony on the consistency part, not just in a single file, but with rest of the codebase. Formalizing this as a preferred guideline (and not as a rule) would help. As Andreas points out, exceptions to the guideline could very well be long function signatures which wrap lines, where a newline would help with readability.

@nalimilan
Copy link
Member

+1 for consistency as well.

@@ -600,7 +600,6 @@ function insert!(B::BitVector, i::Integer, item)
end

function _deleteat!(B::BitVector, i::Integer)

k, j = get_chunks_id(i)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't delete blank lines in cases like this where the author has clearly taken some care to use whitespace to delineate functional blocks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes the code less readable, as it detaches the contents of the function from the surrounding blocks that denote where the function starts and ends. a consistent policy is better than an inconsistent one, even if aesthetic judgements about it differ between contributors.

tkelman added 10 commits April 15, 2016 11:48
remove more extraneous newlines, rearrange signatures for VersionNumber

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
2 in a row is generally enough
Adjust some signatures for uniformity and line length
would get MethodError or InexactError later for non integer inputs, but still
tkelman added 3 commits April 15, 2016 11:48
If these ever get exported, this can be reverted for consistency with
other exported iterator functions which are using a lower case convention
at the moment.

Leaving in place pgenerate since it at least has multiple nontrivial
signatures.
@tkelman tkelman merged commit 6392078 into master Apr 17, 2016
@tkelman tkelman deleted the tk/style branch April 17, 2016 03:57
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.

None yet

7 participants