Remove semicolons from JS tests

[zlatanvasovic]

💛

[ghost]

This is fixed.

[zlatanvasovic]

@twbs Can anybody review this as soon as possible? Every single change in JS unit test files may require start from scratch, so I need to do all the things again. 😟

[mdo]

Given the larger chunks of JS being rewritten, we need @fat to look into this.

[XhmikosR]

Even after setting indent: 2 in jshintrc and fixing indentation for tests/*.js I keep getting warnings for the test files...

[zlatanvasovic]

Hm?

[XhmikosR]

NVM, I fixed the issues myself.

[XhmikosR]

@zdroid: do you mind if I make a new PR for this and then you just rebase for the semicolons?

[zlatanvasovic]

Hm. What was the problem with tests?

2014-02-13 9:09 GMT+01:00 XhmikosR [email protected]:

@zdroid https://github.com/ZDroid: do you mind if I make a new PR for
this and then you just rebase for the semicolons?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/12058#issuecomment-34956189
.

Zlatan Vasović - ZDroid

[XhmikosR]

Well, it simply is easier for me to review my changes. This is one patch with indentation changes + semicolons.

[XhmikosR]

See #12720. After that is in, you can rebase your branch and update the PR to get only the semicolon changes.

[zlatanvasovic]

JSHint can be bad with indent checking, especially when there is a indent
option. We should use JSCS for that.

Can these pull requests be done vice versa? First this for general coding
style then your for the rest? Rebase for 1.2k lines of code rewrite aren't
so easy. :(

You can also get access to ZDroid/bootstrap and add commits directly here,
so nobody needs the rebase. :D

2014-02-13 9:21 GMT+01:00 XhmikosR [email protected]:

See #12720 #12720. After that is
in, you can rebase your branch and update the PR to get only the semicolon
changes.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/12058#issuecomment-34956864
.

Zlatan Vasović - ZDroid

[XhmikosR]

This was much easier for me to do it myself than reviewing this; if you hadn't mixed the indentation changes with the others I wouldn't mind...

[XhmikosR]

BTW, I'm all for not using JSHint's indent option but for now it does the job as you can see from my PR. We can switch to JSCS after that.

[zlatanvasovic]

Hm. I fixed indent, removed semicolons and removed transition support
statements as they're turned of in index.html. Some of these may collide
with yours.

2014-02-13 9:42 GMT+01:00 XhmikosR [email protected]:

BTW, I'm all for not using JSHint's indent option but for now it does the
job as you can see from my PR. We can switch to JSCS after that.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/12058#issuecomment-34958129
.

Zlatan Vasović - ZDroid

[XhmikosR]

I will have a closer look later today and rebase your patch on top of mine.

[zlatanvasovic]

OK.

2014-02-13 9:50 GMT+01:00 XhmikosR [email protected]:

I will have a closer look later today and rebase your patch on top of mine.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/12058#issuecomment-34958686
.

Zlatan Vasović - ZDroid

[XhmikosR]

@zdroid: I checked your changes and I'm not sure they all make sense, so I won't include them in my PR. I suggest you wait until the other PR is merged and then create separate patches for the changes you make. For example, the semicolons change is fine, but some others aren't,

Rebasing wasn't so hard but there will be conflicts ofc.

[XhmikosR]

I also tried JSCS but its indentation check seems weird to me... So I'll definitely leave this for another time.

[zlatanvasovic]

OK so far.

2014-02-13 17:41 GMT+01:00 XhmikosR [email protected]:

I also tried JSCS but its indentation check seems weird to me... So I'll
definitely leave this for another time.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/12058#issuecomment-34997441
.

Zlatan Vasović - ZDroid

[XhmikosR]

I had a closer look and the remaining indentation changes break grunt jshint. Please, rebase and keep the patches separately so that they are easily reviewed.

[zlatanvasovic]

better?

[XhmikosR]

Almost there :P

I think the semicolons removal should be done separately from the transition change.

[XhmikosR]

This should also be separate.

[zlatanvasovic]

Lol. So I should make 3 (or 4) pull requests. :D

Can I just make 2: for semicolons and for the rest?

[XhmikosR]

I meant separate patches, not separate pull requests.

[hnrch02]

I meant separate patches, not separate pull requests.

Just to clarify, basically a single PR with separate commits?

[zlatanvasovic]

I just don't like pull requests with more than one commit. One commit per
pull request, best for merge.

2014-02-17 11:16 GMT+01:00 Heinrich Fenkart [email protected]:

I meant separate patches, not separate pull requests.

Just to clarify, basically a single PR with separate commits?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/12058#issuecomment-35243812
.

Zlatan Vasović - ZDroid

[XhmikosR]

Commits are free; mixing different changes in the same commit is just bad practice.

All these changes refer to the same files; tests. That is why I suggested separate patches in the same PR. But separate PRs are ok I guess, if you insist on keeping one patch per PR.

[zlatanvasovic]

One commit per pull request is pretty clear if you describe what is changed (like I did in the pull request description).

[XhmikosR]

No, it's not. It's too generic patch which can be split up in clear, separate patches.

[zlatanvasovic]

@XhmikosR I broke the pull request into 3 smaller ones.

[XhmikosR]

LGTM now.

[cvrebert]

*rages internally about the semicolon thing generally*

[XhmikosR]

Semicolons, comma first, spaces vs tabs... So many different opinions :P

[zlatanvasovic]

Yay. As @fat wrote the most of Bootstrap JS and Bootstrap docs JS code, he can make opinions about js/ (including js/tests/) and docs/assets/js/.

[XhmikosR]

@zdroid: your last comment is irrelevant. I simply pointed out that there are so many different opinions on the style that things get complex.

[zlatanvasovic]

Yes, but I talked specifically about semicolons. :P

2014-02-18 9:19 GMT+01:00 XhmikosR [email protected]:

@zdroid https://github.com/ZDroid: your last comment is irrelevant. I
simply pointed out that there are so many different opinions on the style
that things get complex.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/12058#issuecomment-35361692
.

Zlatan Vasović - ZDroid

[cvrebert]

@XhmikosR So, gonna merge this?

[XhmikosR]

I guess so since that's the current style already.

[cvrebert]

!