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

lib: change multiline function signatures style #97

Closed
wants to merge 3 commits into from

Conversation

aqrln
Copy link
Member

@aqrln aqrln commented Mar 3, 2017

Make multiline function signatures more concise and readable.

Make multiline function signatures more concise and readable.
@belochub
Copy link
Member

belochub commented Mar 5, 2017

In my opinion, it will be better to use another style, like this one:

Client.prototype.connectAndHandshake = function(appName, username,
    password, callback) {

Or something like that:

Client.prototype.connectAndHandshake = function(appName,
                                                username,
                                                password,
                                                callback) {

The latter style is used in Node.js core and looks more suitable for this case.

@aqrln
Copy link
Member Author

aqrln commented Mar 5, 2017

@belochub I'm all for the latter style but AFAIK @tshemsedinov objects to it. Though I'm fine with the first one too.

@tshemsedinov
Copy link
Member

I like:

Client.prototype.connectAndHandshake = function(
  appName, // comments
  username, // comments
  password, // comments
  callback // comments
) {

@aqrln
Copy link
Member Author

aqrln commented Mar 5, 2017

@tshemsedinov yeah, and that's what we currently have, but that style has appeared to be not very readable and making it very hard to skim through the source code when there's a lot of such functions (like what I am constantly experiencing trying to read MetaSync or GlobalStorage sources) since their bounds are not very evident for the eye (or that may be a matter of habit, IDK). Though if the parameters were indented with four spaces, I'd find it somewhat more readable since they wouldn't meld with the function body.

@belochub what do you think about it?

@belochub
Copy link
Member

belochub commented Mar 5, 2017

@aqrln, I like the way you styled it in last commit (3f831c4).
@tshemsedinov, since we don't add comments on arguments like in the style you like (because we have descriptions in front of functions including detailed argument description), we may use style which @aqrln added in last commit (former of those described here #97 (comment)).

@tshemsedinov
Copy link
Member

If parameters need no explanation we can just:

Client.prototype.connectAndHandshake = function(
  appName, username, password, callback
) {
  // code
}

or comment just certain parameters:

Client.prototype.connectAndHandshake = function(
  appName, username, password,
  callback // function(err, data)
) {
  // code
}

and make comments for function itself (not parameters), add examples, etc.

Client.prototype.connectAndHandshake = function(
  // Comments for function
  appName, username, password, callback
  // Example: ...
) {
  // code
}

As for me, it doesn't meld with body.

@aqrln
Copy link
Member Author

aqrln commented Mar 6, 2017

Then my vote goes for

Client.prototype.connectAndHandshake = function(
  appName, username, password, callback
) {
  // code
}

I'm not really a fan of this style but at least I find it acceptable. So let's use it and close the PR without further discussion since the whole issue is bikeshedding while we have much more important tasks.

aqrln added a commit that referenced this pull request Mar 6, 2017
Make multiline function signatures more concise.

PR-URL: #97
@aqrln
Copy link
Member Author

aqrln commented Mar 6, 2017

Landed in 9edd03f.

@aqrln aqrln closed this Mar 6, 2017
@aqrln aqrln deleted the paramenter-style branch March 6, 2017 23:15
aqrln added a commit that referenced this pull request Mar 13, 2017
Make multiline function signatures more concise.

PR-URL: #97
@aqrln aqrln mentioned this pull request Mar 13, 2017
aqrln added a commit that referenced this pull request Mar 13, 2017
Make multiline function signatures more concise.

PR-URL: #97
belochub pushed a commit that referenced this pull request Jan 22, 2018
Make multiline function signatures more concise.

PR-URL: #97
belochub pushed a commit that referenced this pull request Jan 22, 2018
Make multiline function signatures more concise.

PR-URL: #97
@belochub belochub mentioned this pull request Jan 22, 2018
belochub pushed a commit to metarhia/mdsf that referenced this pull request Jul 19, 2018
Make multiline function signatures more concise.

PR-URL: metarhia/jstp#97
belochub pushed a commit to metarhia/mdsf that referenced this pull request Jul 19, 2018
Make multiline function signatures more concise.

PR-URL: metarhia/jstp#97
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.

3 participants