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

Adopt a unified style for Flot's codebase. #883

Open
dnschnur opened this issue Nov 27, 2012 · 4 comments
Open

Adopt a unified style for Flot's codebase. #883

dnschnur opened this issue Nov 27, 2012 · 4 comments
Assignees
Milestone

Comments

@dnschnur
Copy link
Member

Flot has had many contributions, and the code has become a mish-mash of different styles. Maintaining and contributing to the library would be easier if we adopted a unified code style. The proposed style is described in the contribution guidelines.

@ghost ghost assigned dnschnur Nov 27, 2012
@markrcote
Copy link
Contributor

Good idea to have a coding style, but flot's core and many of the packaged plug-ins don't use the expansive horizontal spacing of the jQuery style. I would propose we stick with the compact horizontal spacing that most files have, just so we don't make it a goal to change huge swathes of existing code.

In other words, I argue that we should continue to do

if (foo(x, y) > z) {

instead of

if ( foo( x, y ) > z ) {

Similarly I would argue against extra vertical spacing, continuing to prefer

function foo() {
    // do some stuff
    // do some more stuff
}

to

function foo() {

    // do some stuff
    // do some more stuff

}

although I see that you have already started modifying some code in that direction... in general I would like to stick with the prevalent style, even if it's not the jQuery standard, just to minimize the number of lines changed merely for stylistic reasons.

@markrcote
Copy link
Contributor

I also think that the preference for 80-character lines (with which I agree) is at odds with having lots of horizontal spacing.

@dnschnur
Copy link
Member Author

dnschnur commented Dec 4, 2012

Right now it's mostly theoretical; I ran a test against the fillbetween plugin, but wasn't planning to do any others until after 0.8 is released. The codebase already has a lot of 'bad practice' issues - missing braces, use of == rather than === for cases other than null, etc. - that I'd really like to fix, so cleaning up style at the same time isn't such a big deal.

I do think that a common style is very valuable, and choosing the jQuery style had less to do with the actual rules than the fact that it's the most widely-adopted and familiar one. The simpler we make it - the less exceptions to the base document - the easier it is for contributors to do the right thing.

You raise a good point about the 80-character width, though, and personally I prefer the narrower horizontal spacing. So I'll add that as an exception - no extra space before or after parens and square brackets. Unless you feel strongly about it, I'd like to keep the vertical spacing, though; I've always found that a little easier to read.

What are your thoughts?

@markrcote
Copy link
Contributor

Yup I agree with everything you said. I'll deal with the extra vertical spacing. :) My main concern was that changing horizontal spacing would touch many, many lines, thus confusing commit history. Vertical spacing doesn't suffer from this problem. So sounds good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants