Code style consistency

[FagnerMartinsBrack]

This pull requests fixes a few jshint violations and make the code-style consistent as close as possible with jquery core style guidelines.

It uses jscs "jquery" preset to be able to validate the code via command line.

The task grunt validate was created to be able to validate the code style outside the grunt build, it removes the burden of contributors having to successfully adhere to the code style in order to make the build pass successfully.

Great care was taken for not changing anything unrelated in order to avoid inadvertent behavior changes. Unfortunately this messes up with the git blame, but consistency is definitely a reasonable tradeoff.

All tests results remain unchanged and all examples are working.

Note: Many points in the code were changed in a "dumb way", like declaring vars in the top of the function scope but not moving the original assignment (to prevent ordering issues), I did that because the tests are not very well covered, so any small typo could have drastic consequences. I really just tried to adhere to the jscs and jshint console errors without any relevant refactoration.

Note2: The package.json file seems more modified than expected due to the fact I used npm install <package> --save-dev instead of manually adding the contents. the only manual change was the "~" prefix, which uses the "0.x.0" update pattern. I do recommend to use "0.0.x" versioning pattern for package.json in order to avoid build breakage, not every author follow the semver spec in the NPM community, so a minor version update may contain backwards compatibility issues.

The next steps I am intended to make after this would be:

1. Take a look in the automatic tests and see if they can be improved, while adding new tests when necessary.
2. Integrate both unix and windows based CI to ensure stability in both platforms.
3. Add additional features after reducing the technical debt of relevant code.
4. Check how is Is better documentation needed? #158 going and improve the documentation (just document what is being tested, avoid documenting untested code).

@shama Please review and tell me what you think about this.

[shama]

Leaving the decision up to @sokra if he sees this as valuable.

In my opinion, I don't see value in these changes. I don't care for jquery core style guides. The combined var statements are less readable among other issues and the white space around parens is noisy. Also I don't know what kind of bugs this may introduce and don't have time review 3000+ line changes, sorry.

[sokra]

Sorry but I don't see any benefit from this. The moved vars look ugly and the spacing is hard to type.

If you want you can run the tool again with different settings. So everything looks like this:

var a = 1;
var b = {
    x: 1,
    y: 2,
};

if(a > 1 && b) {
    callMethod(a, b);
    if(b.x) return;
}

[FagnerMartinsBrack]

I don't care for jquery core style guides. The combined var statements are less readable among other issues and the white space around parens is noisy.

This pull request is not meant to discuss code style rules. The real value is the consistency.

I am aware of Ben Alman multiple var statements and I use that in all of my projects, I don't actually use jquery core style guidelines in its integral form, I do with a few exceptions. But that's not the point, the point here is the following:

Facts

• This is a jquery plugin
• The javascript code is what matters most in this project
• Consistency should exist through an entire codebase to make it easy for a contributor to contribute and others to review code

Solution

• Use jQuery core style guidelines in its integral form in the JS code, so we don't have to worry about it.

The intent here is to prevent subjective opinions. Code style discussions are the ones that really doesn't add any value to the table, consistency does.

So if the decision here is not to adhere to jquery core style guidelines, at least point me exactly which style you are going to use and stick with it throughout the whole codebase, validating with jscs whenever possible.

My intent is just to start contributing, but with an inconsistent codebase it turns out to be impossible.

Also I don't know what kind of bugs this may introduce and don't have time review 3000+ line changes

That's a fair point, but I don't know any other solution other than you do it, and as you stated you don't have time.

There's the option to start being consistent along with each specific new implementation or fix, but then that would make each PR a pain to be reviewed.

Another option is to create a "development branch" and put all these changes on it. Creating beta releases along the way before releasing a new stable version.

[FagnerMartinsBrack]

If you want you can run the tool again with different settings. So everything looks like this:

Alright, document every style you want to change and I will do it. As I said, I am just looking forward consistency, that's all.

[sokra]

Best we use the setting that causes as few changes to the codebase as possible.

[FagnerMartinsBrack]

@sokra

Best we use the setting that causes as few changes to the codebase as possible.

I agree, but since I didn't created the code it would take much more time for me to analyze it. I will comply with your given example and validate it using the jscs tool if you think that would cause few changes in the code. I am also up to document the decided code style in a markdown file to ensure things do not change in the future, ok?

If there's anything else you want me to do just point it. As I mentioned, I am not willing to use this PR to start any code style discussion, I am just willing to start to contribute in a consistent codebase.

[FagnerMartinsBrack]

@shama @sokra

Just one more thing.

Due to the fact the codebase is widely inconsistent this pull request is not trivial, it takes time and I want to be sure I am not doing something that is going to be changed later.

For that reason I commited a markdown file in the CONTRIBUTING section of github to make it clear the rules that should be made consistent: https://github.com/web-stories/jmpress.js/blob/code-style/CONTRIBUTING.md

There are a few questions that I have before proceeding further, just tell me which one to use and I adjust the tool:

• How is the spacing in array literal going to be handled?
• How is the spacing in ternary operator going to be handled?
• How is the spacing in named and anonymous functions going to be handled?
• Should all vars be declared on top?
• Should the line be 80/100 chars long or there's no limit?
• Should unary special-character ++, -- have space or not?
• : after a property name in an object definition must have a single preceding space only?
• Should strings use single quotes or double quotes?
• Semicolon always?
• Should case be aligned with switch keyword?
• Should equality always be strict ===?

Those answers should be enough to enable a minimum initial consistency threshold for decent contributions, after that the codebase could be adapted as it goes.

[shama]

Closing as I think the existing code style is fine for now and don't want to potentially add bugs and destroy the git blame with overzealous sweeping changes. I also don't think we need to create a bunch of rules in order for people to contribute.

If spaces between parentheses is preventing anyone from contributing, they should get over it. There are far more important problems to solve. Thanks anyways.

[FagnerMartinsBrack]

If spaces between parentheses is preventing anyone from contributing, they should get over it. There are far more important problems to solve. Thanks anyways.

It seems you still fail to understand the point, I never complained about code style, you are the one complaining about it. Its about consistency. There are far more important problems to solve in which cannot be done without a decent codebase and clear code comparison. The interesting fact is that consistency doesn't matter for those who already know the codebase, but for whose who are going to read it, it matters a lot. I wonder why there isn't contributions in this repo and some people have trouble to make sense of the written code.

I also never said that a bunch of rules should be created for other people to contribute, the markdown file is just internal documentation. It could be created with another name other than CONTRIBUTING.md if that's the case. The code style enforcement doesn't exist, you just type grunt validate before commiting a pull request and you can fix manually any code style violation without leaving the burden to the contributor.

Anyway, asking for someone to "get over with" in an important matter without proposing a solution when you keep tagging issues with "Please contribute" seems pretty ironic.