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

Declare leading-commas to be consistent in style. #1

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cstumph
Copy link

@cstumph cstumph commented Mar 17, 2015

No description provided.

@cstumph
Copy link
Author

cstumph commented Mar 17, 2015

We currently have 2 indentation inconsistencies:

  • indenting the leading comma VS not indenting
  • consecutive vars VS obj properties.

This addresses the first inconsistency. It was requested I address the second as part of this pull request.
I've not had an issue with the second inconsistency, nor have I noticed others having an issue with it, so I can't say I'm comfortable making additional impositions on the issue.
Personally I'm okay with a difference existing between two functionally different pieces of syntax.
My main axe to grind here is ease of readability regarding object composition, preferably not at the cost of consecutive var statement readability IMO.

Furthermore, there's stuff like when people drop to a new line doing long chained function calls, etc. We can say "don't do that" but it can and will happen sometimes. So biting off more here, I think, has potential to go even further...

@cstumph
Copy link
Author

cstumph commented Mar 17, 2015

Btw, example of this in our codebase. Doesn't seem to cause issue. Seemingly no struggles because of it:

var Base = require('ribcage-view')
  , _ = require('lodash')
  , Button = require('../../pieces/button')
  , ProfileEdit = require('../profile-edit')
  , SettingsContractorStep = require('./step')
  , currentUser = require('../../../app/current-user.js')
  , dataTypeBackboneModel = require('../../mixins/dataType-backbone-model')
  , State = require('ampersand-state').extend({
      extraProperties: 'reject'
    , toJSON: function toJSON(){
        return _.extend(this.serialize(), this.getAttributes({derived: true}))
      }
    }).extend(dataTypeBackboneModel())

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.

1 participant