Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

Added space in function definitions#488

Closed
PierBover wants to merge 1 commit intoatom:masterfrom
PierBover:master
Closed

Added space in function definitions#488
PierBover wants to merge 1 commit intoatom:masterfrom
PierBover:master

Conversation

@PierBover
Copy link
Copy Markdown

@PierBover PierBover commented Feb 6, 2017

To be more consistent with the other snippets in this package I've added a space in some function definitions that lacked it.

For example if (...) { and while (...) { have a space between the keyword and the parenthesis but some function definitions did not and resulted in function foo() {.

This behaviour is inconsistent. For example some function definitions did have the space such as:

  'Object Method':
    'prefix': 'kf'
    'body': '${1:methodName}: function (${2:attribute}) {\n\t$3\n}${4:,}'

Or also:

  'Prototype':
    'prefix': 'proto'
    'body': '${1:ClassName}.prototype.${2:methodName} = function ($3) {\n\t$0\n};'

But not in:

  'Function':
    'prefix': 'fun'
    'body': 'function ${1:functionName}($2) {\n\t$0\n}'

With the new changes all function definitions get the space so the result is function foo () { and function () {.

@PierBover
Added space in function definitions
f4cb51d 
To be more consistent with the other snippets I've added a space in function definitions.
@PierBover
Copy link
Copy Markdown
Author

PierBover commented Feb 10, 2017

Any comments @50Wliu or @lee-dohm ?

@lee-dohm
Copy link
Copy Markdown
Contributor

lee-dohm commented Feb 10, 2017

Thanks for the suggestion! This is changing a default, so people that are using the current snippets and like them the way they are (or gotten used to them) will have their workflow broken. They'll have to create new snippets to maintain their current workflow. We tend to be very careful around changing the defaults on people, so there is already a high bar here.

Regarding consistency, if and while aren't function definitions, so I don't consider them to be evidence of a consistency problem. "Object Method" and "Prototype" are both assigning anonymous functions to a name, rather than creating named functions, so structurally they're different. The only place that I have seen the "function name separate from the parameter list" enforced is in standard.js and while standard.js is used by Atom and Electron and somewhat common besides, it isn't universal.

I don't feel there's enough motivation here to justify changing a default. I'll pass it by the team and see if anyone else has any input first though.

@PierBover
Copy link
Copy Markdown
Author

PierBover commented Feb 11, 2017

Thanks @lee-dohm I understand. Yeah maybe I'm being too OCD :)

Let's see if anyone on the team agrees.

@lee-dohm
Copy link
Copy Markdown
Contributor

lee-dohm commented Feb 11, 2017

Thanks for understanding! We're all pedantic in our own ways 😆

@PierBover
Copy link
Copy Markdown
Author

PierBover commented Mar 9, 2017

Hey @lee-dohm I'm guessing I should just close this PR, right?

@lee-dohm
Copy link
Copy Markdown
Contributor

lee-dohm commented Mar 13, 2017

@PierBover Yes, thank you. My apologies for not getting back to you on it.

@lee-dohm lee-dohm closed this Mar 13, 2017
@lee-dohm
Copy link
Copy Markdown
Contributor

lee-dohm commented Mar 13, 2017

Thanks very much for the effort you put in to this @PierBover ❤️

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

needs-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@PierBover @lee-dohm @winstliu