+ mobile-l
This is all useful feedback. That would be ideal if jscs had this capability but an interim solution would be good in the meantime.
I think it's important to codify our coding conventions across the project. It will make it much easier for newbies to write code without having to worry about coding style.
On Fri, Nov 21, 2014 at 11:19 PM, Krinkle krinklemail@gmail.com wrote:
Hi Jon,
Just wanted to quickly share my ideas on code formatting.
First off, as long as there are no side effects (e.g. normalising too much), any tool will do in the mean-while and it's trivial to switch later on (e.g. just change which tasks the "grunt fixup" alias will run). They wouldn't be run as part of "grunt test". Instead it's a convenience tool for developers to easily reformat code locally before submission (e.g. after jscs pointed out one or more errors in their local grunt run). This is among the reasons why using a local Grunt is so convenient. It enables individual projects to try out new tools without requiring server-side configuration. I try out new tasks all the time in oojs-core and VisualEditor. Some stick, some don't.
Having said that, I've used js-beautifier in the past and worry it won't work well for us.
esformatter[1] on the other hand seems to have a more stable implementation and general approach.
However, any tool other than jscs will come with the down side of having to declare your style guide, again. Which will amount to tedious duplication and loads of edge cases where the tools declare the style using different logical rules and will never match.
jscs ships with a wikimedia preset that saves lots of configuration in the first place.
And fortunately, jscs actually plans to ship an "autofixer" utility that will correct violations from within jscs itself. Using the solid parser and tokeniser that jscs is known for. Thus, even the small jscs rule config we do have, won't have to be duplicated.
The jQuery Team also supports this effort for their various javascript projects (both as style checker, as we do already, and as code formatter). So that's a major player we'll have on our side when it comes to continued support for the tool.
https://github.com/jscs-dev/node-jscs/issues/516
Supported by Mike Sherov (mikesherov) and Oleg Gaidarenko (markelog), from the jQuery Team (who are also jscs collaborators). And Marat Dulin (mdevils), founder of jscs.
Best,
— Krinkle
[1] https://github.com/millermedeiros/esformatter
On 20 Nov 2014, at 23:57, Derk-Jan Hartman d.j.hartman@gmail.com wrote:
Begin forwarded message:
Date: 20 november 2014 21:19:24 CET From: Jon Robson jrobson@wikimedia.org To: Bahodir Mansurov bmansurov@wikimedia.org Cc: mobile-l mobile-l@lists.wikimedia.org Subject: Re: [WikimediaMobile] Using jsbeautify in MobileFrontend
Follow up If I run `make jsbeautify` now then https://gist.github.com/jdlrobson/a05ddad00175ebceac68 is the result.
Outstanding actions:
- Need input on rest of the team about which of delete
this.cache[title]; or delete this.cache[ title ]; is the preferred standard.
- You'll notice jsbeautify and inlne comments need to be ironed out.
For example: https://gist.github.com/jdlrobson/a05ddad00175ebceac68#file-gistfile1-diff-L...
Apart from the above 2 jsbeautify makes some adjustments to our whitspace which I guess we'll just have to live with.
Please can everyone else let me know how they think we should proceed?
On Wed, Nov 19, 2014 at 4:12 AM, Bahodir Mansurov bmansurov@wikimedia.org wrote:
As for # Rule 4, it makes sense to add spaces inside square brackets. The reasoning is the same as why we add spaces inside parenthesis.
On Nov 18, 2014, at 12:28 PM, Jon Robson jrobson@wikimedia.org wrote:
I explored running jsbeautify [1] on the MobileFrontend codebase and looked at how the output differs from the current styling. It introduces various rules that MobileFrontend is currently not adhering too. MobileFrontend already uses jscs [2] so we want to make sure the outputs of both are compatible. Here is my report on that with the recommendation that we should use it.
#Rule 1: object properties defined on a single line. e.g. { foo: 2, bar: 3 } NOT { foo: 2, bar: 3 }
I think this would be a good idea to adopt. I will explore if jscs can enforce this.
# Rule 2: variables that are initialised must be followed by a new line (although I noted a few odd cases e.g. in Page.js after a "?:" expression and /MobileWebClickTracking.js e.g. var Foo, bar = $( 'div' ), Baz, Bar;
not: var Foo, bar = $( 'div' ), Baz, Bar;
This will be fixed if I implement https://trello.com/c/0dkx0ldL
# Rule 3: All chained events should be indented e.g. foo() .bar();
not foo(). bar();
Seems like a no brainer. One that happens naturally most of the time.
# Rule 4: Spacing in object parameters e.g. foo[ 1 ] [ 1, 2, 3, 4 ]
not: foo[1] [1, 2, 3, 4]
This is different to MediaWiki coding conventions but I can implement https://github.com/beautify-web/js-beautify/issues/424 to give us this. We seem a bit inconsistent ourselves with this convention - let me know how you think this rule should work in our codebase...
# Rule 5: New line after variable declarations
var x, y, z;
z();
not: var x, y, z; z();
Also: function foo() {}
function bar() {}
not: function foo() {} function bar() {}
Seems like a no brainer and shouldn't introduce any major issues with code review.
# Rule 6: Comments must respect the current indentation level
if () { ... // If i is 5 we do something special } else if ( i === 5 ){
} becomes if () { ... // If i is 5 we do something special } else if ( i === 5 ){
} We'll have to think about what to do with these comments but I don't think this should be a blocker to using jsbeautify. It will only pop up occasionally.
# Rule 7: On long if statements the curly brace must be indented. And the first condition should be on the same line
if ( enableToc || ... .... && baz ) {
not: if ( enableToc || ... .... && baz ) {
Again I'm not sure if this will happen too often. This to me is a sign that we should be using functions rather than long if statements personally. Again not a blocker.
Actions:
- Implement https://github.com/beautify-web/js-beautify/issues/424
- You guys need to advise me on how we should handle square brackets
in our codebase in such a way we respect MediaWiki coding conventions and come up with a consistent style we are happy with and can adhere to.
- I'll implement https://trello.com/c/0dkx0ldL in some form
- I'll explore if jscs can support objects defined on new line
- When the above are done I recommend we turn on jsbeautify for the project.
I've setup a tracking card for the above work: https://trello.com/c/5btWf2JN/90-snakes-on-a-plane and will be looking at these problems today.
[1] https://github.com/beautify-web/js-beautify [2] https://github.com/jscs-dev/node-jscs
Mobile-l mailing list Mobile-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mobile-l
Mobile-l mailing list Mobile-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mobile-l
Personally, I like hideOverlay( this.stack[0].overlay );
Having spaces inside parentheses makes sense since parameters are usually words or word-like and it's easier to read words when they have spaces around them. Keys inside brackets are often just numbers, which I don't think really benefit from having spaces. That said, I realize this is inconsistent and I would be OK with using spaces for both. Not using spaces at all makes it harder to scan code for particular variables, etc, IMO.
Kaldari
On Fri, Nov 21, 2014 at 8:56 AM, Jon Robson jrobson@wikimedia.org wrote:
- mobile-l
This is all useful feedback. That would be ideal if jscs had this capability but an interim solution would be good in the meantime.
I think it's important to codify our coding conventions across the project. It will make it much easier for newbies to write code without having to worry about coding style.
On Fri, Nov 21, 2014 at 11:19 PM, Krinkle krinklemail@gmail.com wrote:
Hi Jon,
Just wanted to quickly share my ideas on code formatting.
First off, as long as there are no side effects (e.g. normalising too
much),
any tool will do in the mean-while and it's trivial to switch later on
(e.g.
just change which tasks the "grunt fixup" alias will run). They wouldn't
be
run as part of "grunt test". Instead it's a convenience tool for
developers
to easily reformat code locally before submission (e.g. after jscs
pointed
out one or more errors in their local grunt run). This is among the
reasons
why using a local Grunt is so convenient. It enables individual projects
to
try out new tools without requiring server-side configuration. I try out
new
tasks all the time in oojs-core and VisualEditor. Some stick, some don't.
Having said that, I've used js-beautifier in the past and worry it won't work well for us.
esformatter[1] on the other hand seems to have a more stable
implementation
and general approach.
However, any tool other than jscs will come with the down side of having
to
declare your style guide, again. Which will amount to tedious duplication and loads of edge cases where the tools declare the style using different logical rules and will never match.
jscs ships with a wikimedia preset that saves lots of configuration in
the
first place.
And fortunately, jscs actually plans to ship an "autofixer" utility that will correct violations from within jscs itself. Using the solid parser
and
tokeniser that jscs is known for. Thus, even the small jscs rule config
we
do have, won't have to be duplicated.
The jQuery Team also supports this effort for their various javascript projects (both as style checker, as we do already, and as code
formatter).
So that's a major player we'll have on our side when it comes to
continued
support for the tool.
https://github.com/jscs-dev/node-jscs/issues/516
Supported by Mike Sherov (mikesherov) and Oleg Gaidarenko (markelog),
from
the jQuery Team (who are also jscs collaborators). And Marat Dulin (mdevils), founder of jscs.
Best,
— Krinkle
[1] https://github.com/millermedeiros/esformatter
On 20 Nov 2014, at 23:57, Derk-Jan Hartman d.j.hartman@gmail.com
wrote:
Begin forwarded message:
Date: 20 november 2014 21:19:24 CET From: Jon Robson jrobson@wikimedia.org To: Bahodir Mansurov bmansurov@wikimedia.org Cc: mobile-l mobile-l@lists.wikimedia.org Subject: Re: [WikimediaMobile] Using jsbeautify in MobileFrontend
Follow up If I run `make jsbeautify` now then https://gist.github.com/jdlrobson/a05ddad00175ebceac68 is the
result.
Outstanding actions:
- Need input on rest of the team about which of delete
this.cache[title]; or delete this.cache[ title ]; is the preferred standard.
- You'll notice jsbeautify and inlne comments need to be ironed out.
For example:
https://gist.github.com/jdlrobson/a05ddad00175ebceac68#file-gistfile1-diff-L...
Apart from the above 2 jsbeautify makes some adjustments to our whitspace which I guess we'll just have to live with.
Please can everyone else let me know how they think we should proceed?
On Wed, Nov 19, 2014 at 4:12 AM, Bahodir Mansurov bmansurov@wikimedia.org wrote:
As for # Rule 4, it makes sense to add spaces inside square brackets. The reasoning is the same as why we add spaces inside parenthesis.
On Nov 18, 2014, at 12:28 PM, Jon Robson jrobson@wikimedia.org wrote:
I explored running jsbeautify [1] on the MobileFrontend codebase and looked at how the output differs from the current styling. It introduces various rules that MobileFrontend is currently not adhering too. MobileFrontend already uses jscs [2] so we want to make sure the outputs of both are compatible. Here is my report on that with the recommendation that we should use it.
#Rule 1: object properties defined on a single line. e.g. { foo: 2, bar: 3 } NOT { foo: 2, bar: 3 }
I think this would be a good idea to adopt. I will explore if jscs can enforce this.
# Rule 2: variables that are initialised must be followed by a new line (although I noted a few odd cases e.g. in Page.js after a "?:" expression and /MobileWebClickTracking.js e.g. var Foo, bar = $( 'div' ), Baz, Bar;
not: var Foo, bar = $( 'div' ), Baz, Bar;
This will be fixed if I implement https://trello.com/c/0dkx0ldL
# Rule 3: All chained events should be indented e.g. foo() .bar();
not foo(). bar();
Seems like a no brainer. One that happens naturally most of the time.
# Rule 4: Spacing in object parameters e.g. foo[ 1 ] [ 1, 2, 3, 4 ]
not: foo[1] [1, 2, 3, 4]
This is different to MediaWiki coding conventions but I can implement https://github.com/beautify-web/js-beautify/issues/424 to give us this. We seem a bit inconsistent ourselves with this convention - let me know how you think this rule should work in our codebase...
# Rule 5: New line after variable declarations
var x, y, z;
z();
not: var x, y, z; z();
Also: function foo() {}
function bar() {}
not: function foo() {} function bar() {}
Seems like a no brainer and shouldn't introduce any major issues with code review.
# Rule 6: Comments must respect the current indentation level
if () { ... // If i is 5 we do something special } else if ( i === 5 ){
} becomes if () { ... // If i is 5 we do something special } else if ( i === 5 ){
} We'll have to think about what to do with these comments but I don't think this should be a blocker to using jsbeautify. It will only pop up occasionally.
# Rule 7: On long if statements the curly brace must be indented. And the first condition should be on the same line
if ( enableToc || ... .... && baz ) {
not: if ( enableToc || ... .... && baz ) {
Again I'm not sure if this will happen too often. This to me is a sign that we should be using functions rather than long if statements personally. Again not a blocker.
Actions:
- Implement https://github.com/beautify-web/js-beautify/issues/424
- You guys need to advise me on how we should handle square brackets
in our codebase in such a way we respect MediaWiki coding conventions and come up with a consistent style we are happy with and can adhere to.
- I'll implement https://trello.com/c/0dkx0ldL in some form
- I'll explore if jscs can support objects defined on new line
- When the above are done I recommend we turn on jsbeautify for the
project.
I've setup a tracking card for the above work: https://trello.com/c/5btWf2JN/90-snakes-on-a-plane and will be looking at these problems today.
[1] https://github.com/beautify-web/js-beautify [2] https://github.com/jscs-dev/node-jscs
Mobile-l mailing list Mobile-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mobile-l
Mobile-l mailing list Mobile-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mobile-l
Mobile-l mailing list Mobile-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mobile-l
I think we can conclude if jscs doesn't complain then it is not an established coding convention and we should leave that up to the implementer.
Going forward I would suggest in code review we do not nit pick on the things that jscs does not complain about, so if Joaquin wants to run jsbeautify on his pre-commit hook on new patches and it creates code that makes jscs happy he should be allowed to. If there is something that you believe should be a coding convention enabling it in jscs config if the rule exists or write a new rule for jscs.
If we codify more conventions in jscs then tools like jsbeautify will have to be adapted to respect that.
All in favour?
On Sat, Nov 22, 2014 at 2:34 AM, Ryan Kaldari rkaldari@wikimedia.org wrote:
Personally, I like hideOverlay( this.stack[0].overlay );
Having spaces inside parentheses makes sense since parameters are usually words or word-like and it's easier to read words when they have spaces around them. Keys inside brackets are often just numbers, which I don't think really benefit from having spaces. That said, I realize this is inconsistent and I would be OK with using spaces for both. Not using spaces at all makes it harder to scan code for particular variables, etc, IMO.
Kaldari
On Fri, Nov 21, 2014 at 8:56 AM, Jon Robson jrobson@wikimedia.org wrote:
- mobile-l
This is all useful feedback. That would be ideal if jscs had this capability but an interim solution would be good in the meantime.
I think it's important to codify our coding conventions across the project. It will make it much easier for newbies to write code without having to worry about coding style.
On Fri, Nov 21, 2014 at 11:19 PM, Krinkle krinklemail@gmail.com wrote:
Hi Jon,
Just wanted to quickly share my ideas on code formatting.
First off, as long as there are no side effects (e.g. normalising too much), any tool will do in the mean-while and it's trivial to switch later on (e.g. just change which tasks the "grunt fixup" alias will run). They wouldn't be run as part of "grunt test". Instead it's a convenience tool for developers to easily reformat code locally before submission (e.g. after jscs pointed out one or more errors in their local grunt run). This is among the reasons why using a local Grunt is so convenient. It enables individual projects to try out new tools without requiring server-side configuration. I try out new tasks all the time in oojs-core and VisualEditor. Some stick, some don't.
Having said that, I've used js-beautifier in the past and worry it won't work well for us.
esformatter[1] on the other hand seems to have a more stable implementation and general approach.
However, any tool other than jscs will come with the down side of having to declare your style guide, again. Which will amount to tedious duplication and loads of edge cases where the tools declare the style using different logical rules and will never match.
jscs ships with a wikimedia preset that saves lots of configuration in the first place.
And fortunately, jscs actually plans to ship an "autofixer" utility that will correct violations from within jscs itself. Using the solid parser and tokeniser that jscs is known for. Thus, even the small jscs rule config we do have, won't have to be duplicated.
The jQuery Team also supports this effort for their various javascript projects (both as style checker, as we do already, and as code formatter). So that's a major player we'll have on our side when it comes to continued support for the tool.
https://github.com/jscs-dev/node-jscs/issues/516
Supported by Mike Sherov (mikesherov) and Oleg Gaidarenko (markelog), from the jQuery Team (who are also jscs collaborators). And Marat Dulin (mdevils), founder of jscs.
Best,
— Krinkle
[1] https://github.com/millermedeiros/esformatter
On 20 Nov 2014, at 23:57, Derk-Jan Hartman d.j.hartman@gmail.com wrote:
Begin forwarded message:
Date: 20 november 2014 21:19:24 CET From: Jon Robson jrobson@wikimedia.org To: Bahodir Mansurov bmansurov@wikimedia.org Cc: mobile-l mobile-l@lists.wikimedia.org Subject: Re: [WikimediaMobile] Using jsbeautify in MobileFrontend
Follow up If I run `make jsbeautify` now then https://gist.github.com/jdlrobson/a05ddad00175ebceac68 is the result.
Outstanding actions:
- Need input on rest of the team about which of delete
this.cache[title]; or delete this.cache[ title ]; is the preferred standard.
- You'll notice jsbeautify and inlne comments need to be ironed out.
For example:
https://gist.github.com/jdlrobson/a05ddad00175ebceac68#file-gistfile1-diff-L...
Apart from the above 2 jsbeautify makes some adjustments to our whitspace which I guess we'll just have to live with.
Please can everyone else let me know how they think we should proceed?
On Wed, Nov 19, 2014 at 4:12 AM, Bahodir Mansurov bmansurov@wikimedia.org wrote:
As for # Rule 4, it makes sense to add spaces inside square brackets. The reasoning is the same as why we add spaces inside parenthesis.
On Nov 18, 2014, at 12:28 PM, Jon Robson jrobson@wikimedia.org wrote:
I explored running jsbeautify [1] on the MobileFrontend codebase and looked at how the output differs from the current styling. It introduces various rules that MobileFrontend is currently not adhering too. MobileFrontend already uses jscs [2] so we want to make sure the outputs of both are compatible. Here is my report on that with the recommendation that we should use it.
#Rule 1: object properties defined on a single line. e.g. { foo: 2, bar: 3 } NOT { foo: 2, bar: 3 }
I think this would be a good idea to adopt. I will explore if jscs can enforce this.
# Rule 2: variables that are initialised must be followed by a new line (although I noted a few odd cases e.g. in Page.js after a "?:" expression and /MobileWebClickTracking.js e.g. var Foo, bar = $( 'div' ), Baz, Bar;
not: var Foo, bar = $( 'div' ), Baz, Bar;
This will be fixed if I implement https://trello.com/c/0dkx0ldL
# Rule 3: All chained events should be indented e.g. foo() .bar();
not foo(). bar();
Seems like a no brainer. One that happens naturally most of the time.
# Rule 4: Spacing in object parameters e.g. foo[ 1 ] [ 1, 2, 3, 4 ]
not: foo[1] [1, 2, 3, 4]
This is different to MediaWiki coding conventions but I can implement https://github.com/beautify-web/js-beautify/issues/424 to give us this. We seem a bit inconsistent ourselves with this convention - let me know how you think this rule should work in our codebase...
# Rule 5: New line after variable declarations
var x, y, z;
z();
not: var x, y, z; z();
Also: function foo() {}
function bar() {}
not: function foo() {} function bar() {}
Seems like a no brainer and shouldn't introduce any major issues with code review.
# Rule 6: Comments must respect the current indentation level
if () { ... // If i is 5 we do something special } else if ( i === 5 ){
} becomes if () { ... // If i is 5 we do something special } else if ( i === 5 ){
} We'll have to think about what to do with these comments but I don't think this should be a blocker to using jsbeautify. It will only pop up occasionally.
# Rule 7: On long if statements the curly brace must be indented. And the first condition should be on the same line
if ( enableToc || ... .... && baz ) {
not: if ( enableToc || ... .... && baz ) {
Again I'm not sure if this will happen too often. This to me is a sign that we should be using functions rather than long if statements personally. Again not a blocker.
Actions:
- Implement https://github.com/beautify-web/js-beautify/issues/424
- You guys need to advise me on how we should handle square brackets
in our codebase in such a way we respect MediaWiki coding conventions and come up with a consistent style we are happy with and can adhere to.
- I'll implement https://trello.com/c/0dkx0ldL in some form
- I'll explore if jscs can support objects defined on new line
- When the above are done I recommend we turn on jsbeautify for the
project.
I've setup a tracking card for the above work: https://trello.com/c/5btWf2JN/90-snakes-on-a-plane and will be looking at these problems today.
[1] https://github.com/beautify-web/js-beautify [2] https://github.com/jscs-dev/node-jscs
Mobile-l mailing list Mobile-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mobile-l
Mobile-l mailing list Mobile-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mobile-l
Mobile-l mailing list Mobile-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mobile-l
If you run jsbeautify with space_in_paren: false, does that mean that it removes spaces or that it ignores whether or not there are spaces? If the later, I would vote that we do that.
Kaldari
On Fri, Nov 21, 2014 at 10:42 AM, Jon Robson jrobson@wikimedia.org wrote:
I think we can conclude if jscs doesn't complain then it is not an established coding convention and we should leave that up to the implementer.
Going forward I would suggest in code review we do not nit pick on the things that jscs does not complain about, so if Joaquin wants to run jsbeautify on his pre-commit hook on new patches and it creates code that makes jscs happy he should be allowed to. If there is something that you believe should be a coding convention enabling it in jscs config if the rule exists or write a new rule for jscs.
If we codify more conventions in jscs then tools like jsbeautify will have to be adapted to respect that.
All in favour?
On Sat, Nov 22, 2014 at 2:34 AM, Ryan Kaldari rkaldari@wikimedia.org wrote:
Personally, I like hideOverlay( this.stack[0].overlay );
Having spaces inside parentheses makes sense since parameters are usually words or word-like and it's easier to read words when they have spaces around them. Keys inside brackets are often just numbers, which I don't think really benefit from having spaces. That said, I realize this is inconsistent and I would be OK with using spaces for both. Not using
spaces
at all makes it harder to scan code for particular variables, etc, IMO.
Kaldari
On Fri, Nov 21, 2014 at 8:56 AM, Jon Robson jrobson@wikimedia.org
wrote:
- mobile-l
This is all useful feedback. That would be ideal if jscs had this capability but an interim solution would be good in the meantime.
I think it's important to codify our coding conventions across the project. It will make it much easier for newbies to write code without having to worry about coding style.
On Fri, Nov 21, 2014 at 11:19 PM, Krinkle krinklemail@gmail.com
wrote:
Hi Jon,
Just wanted to quickly share my ideas on code formatting.
First off, as long as there are no side effects (e.g. normalising too much), any tool will do in the mean-while and it's trivial to switch later on (e.g. just change which tasks the "grunt fixup" alias will run). They
wouldn't
be run as part of "grunt test". Instead it's a convenience tool for developers to easily reformat code locally before submission (e.g. after jscs pointed out one or more errors in their local grunt run). This is among the reasons why using a local Grunt is so convenient. It enables individual
projects
to try out new tools without requiring server-side configuration. I try
out
new tasks all the time in oojs-core and VisualEditor. Some stick, some don't.
Having said that, I've used js-beautifier in the past and worry it
won't
work well for us.
esformatter[1] on the other hand seems to have a more stable implementation and general approach.
However, any tool other than jscs will come with the down side of
having
to declare your style guide, again. Which will amount to tedious duplication and loads of edge cases where the tools declare the style using different logical rules and will never match.
jscs ships with a wikimedia preset that saves lots of configuration in the first place.
And fortunately, jscs actually plans to ship an "autofixer" utility
that
will correct violations from within jscs itself. Using the solid
parser
and tokeniser that jscs is known for. Thus, even the small jscs rule
config
we do have, won't have to be duplicated.
The jQuery Team also supports this effort for their various javascript projects (both as style checker, as we do already, and as code formatter). So that's a major player we'll have on our side when it comes to continued support for the tool.
https://github.com/jscs-dev/node-jscs/issues/516
Supported by Mike Sherov (mikesherov) and Oleg Gaidarenko (markelog), from the jQuery Team (who are also jscs collaborators). And Marat Dulin (mdevils), founder of jscs.
Best,
— Krinkle
[1] https://github.com/millermedeiros/esformatter
On 20 Nov 2014, at 23:57, Derk-Jan Hartman d.j.hartman@gmail.com wrote:
Begin forwarded message:
Date: 20 november 2014 21:19:24 CET From: Jon Robson jrobson@wikimedia.org To: Bahodir Mansurov bmansurov@wikimedia.org Cc: mobile-l mobile-l@lists.wikimedia.org Subject: Re: [WikimediaMobile] Using jsbeautify in MobileFrontend
Follow up If I run `make jsbeautify` now then https://gist.github.com/jdlrobson/a05ddad00175ebceac68 is the result.
Outstanding actions:
- Need input on rest of the team about which of delete
this.cache[title]; or delete this.cache[ title ]; is the preferred standard.
- You'll notice jsbeautify and inlne comments need to be ironed out.
For example:
https://gist.github.com/jdlrobson/a05ddad00175ebceac68#file-gistfile1-diff-L...
Apart from the above 2 jsbeautify makes some adjustments to our whitspace which I guess we'll just have to live with.
Please can everyone else let me know how they think we should proceed?
On Wed, Nov 19, 2014 at 4:12 AM, Bahodir Mansurov bmansurov@wikimedia.org wrote:
As for # Rule 4, it makes sense to add spaces inside square brackets. The reasoning is the same as why we add spaces inside parenthesis.
On Nov 18, 2014, at 12:28 PM, Jon Robson jrobson@wikimedia.org
wrote:
I explored running jsbeautify [1] on the MobileFrontend codebase and looked at how the output differs from the current styling. It introduces various rules that MobileFrontend is currently not adhering too. MobileFrontend already uses jscs [2] so we want to make sure the outputs of both are compatible. Here is my report on that with the recommendation that we should use it.
#Rule 1: object properties defined on a single line. e.g. { foo: 2, bar: 3 } NOT { foo: 2, bar: 3 }
I think this would be a good idea to adopt. I will explore if jscs can enforce this.
# Rule 2: variables that are initialised must be followed by a new line (although I noted a few odd cases e.g. in Page.js after a "?:" expression and /MobileWebClickTracking.js e.g. var Foo, bar = $( 'div' ), Baz, Bar;
not: var Foo, bar = $( 'div' ), Baz, Bar;
This will be fixed if I implement https://trello.com/c/0dkx0ldL
# Rule 3: All chained events should be indented e.g. foo() .bar();
not foo(). bar();
Seems like a no brainer. One that happens naturally most of the time.
# Rule 4: Spacing in object parameters e.g. foo[ 1 ] [ 1, 2, 3, 4 ]
not: foo[1] [1, 2, 3, 4]
This is different to MediaWiki coding conventions but I can implement https://github.com/beautify-web/js-beautify/issues/424 to give us this. We seem a bit inconsistent ourselves with this convention - let me know how you think this rule should work in our codebase...
# Rule 5: New line after variable declarations
var x, y, z;
z();
not: var x, y, z; z();
Also: function foo() {}
function bar() {}
not: function foo() {} function bar() {}
Seems like a no brainer and shouldn't introduce any major issues with code review.
# Rule 6: Comments must respect the current indentation level
if () { ... // If i is 5 we do something special } else if ( i === 5 ){
} becomes if () { ... // If i is 5 we do something special } else if ( i === 5 ){
} We'll have to think about what to do with these comments but I don't think this should be a blocker to using jsbeautify. It will only pop up occasionally.
# Rule 7: On long if statements the curly brace must be indented. And the first condition should be on the same line
if ( enableToc || ... .... && baz ) {
not: if ( enableToc || ... .... && baz ) {
Again I'm not sure if this will happen too often. This to me is a sign that we should be using functions rather than long if statements personally. Again not a blocker.
Actions:
- Implement https://github.com/beautify-web/js-beautify/issues/424
- You guys need to advise me on how we should handle square brackets
in our codebase in such a way we respect MediaWiki coding conventions and come up with a consistent style we are happy with and can adhere to.
- I'll implement https://trello.com/c/0dkx0ldL in some form
- I'll explore if jscs can support objects defined on new line
- When the above are done I recommend we turn on jsbeautify for the
project.
I've setup a tracking card for the above work: https://trello.com/c/5btWf2JN/90-snakes-on-a-plane and will be looking at these problems today.
[1] https://github.com/beautify-web/js-beautify [2] https://github.com/jscs-dev/node-jscs
Mobile-l mailing list Mobile-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mobile-l
Mobile-l mailing list Mobile-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mobile-l
Mobile-l mailing list Mobile-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mobile-l
From what I understand false means it will remove extra spaces and true
means it will add them if missing. El 21/11/2014 20:39, "Ryan Kaldari" rkaldari@wikimedia.org escribió:
If you run jsbeautify with space_in_paren: false, does that mean that it removes spaces or that it ignores whether or not there are spaces? If the later, I would vote that we do that.
Kaldari
On Fri, Nov 21, 2014 at 10:42 AM, Jon Robson jrobson@wikimedia.org wrote:
I think we can conclude if jscs doesn't complain then it is not an established coding convention and we should leave that up to the implementer.
Going forward I would suggest in code review we do not nit pick on the things that jscs does not complain about, so if Joaquin wants to run jsbeautify on his pre-commit hook on new patches and it creates code that makes jscs happy he should be allowed to. If there is something that you believe should be a coding convention enabling it in jscs config if the rule exists or write a new rule for jscs.
If we codify more conventions in jscs then tools like jsbeautify will have to be adapted to respect that.
All in favour?
On Sat, Nov 22, 2014 at 2:34 AM, Ryan Kaldari rkaldari@wikimedia.org wrote:
Personally, I like hideOverlay( this.stack[0].overlay );
Having spaces inside parentheses makes sense since parameters are
usually
words or word-like and it's easier to read words when they have spaces around them. Keys inside brackets are often just numbers, which I don't think really benefit from having spaces. That said, I realize this is inconsistent and I would be OK with using spaces for both. Not using
spaces
at all makes it harder to scan code for particular variables, etc, IMO.
Kaldari
On Fri, Nov 21, 2014 at 8:56 AM, Jon Robson jrobson@wikimedia.org
wrote:
- mobile-l
This is all useful feedback. That would be ideal if jscs had this capability but an interim solution would be good in the meantime.
I think it's important to codify our coding conventions across the project. It will make it much easier for newbies to write code without having to worry about coding style.
On Fri, Nov 21, 2014 at 11:19 PM, Krinkle krinklemail@gmail.com
wrote:
Hi Jon,
Just wanted to quickly share my ideas on code formatting.
First off, as long as there are no side effects (e.g. normalising too much), any tool will do in the mean-while and it's trivial to switch later
on
(e.g. just change which tasks the "grunt fixup" alias will run). They
wouldn't
be run as part of "grunt test". Instead it's a convenience tool for developers to easily reformat code locally before submission (e.g. after jscs pointed out one or more errors in their local grunt run). This is among the reasons why using a local Grunt is so convenient. It enables individual
projects
to try out new tools without requiring server-side configuration. I try
out
new tasks all the time in oojs-core and VisualEditor. Some stick, some don't.
Having said that, I've used js-beautifier in the past and worry it
won't
work well for us.
esformatter[1] on the other hand seems to have a more stable implementation and general approach.
However, any tool other than jscs will come with the down side of
having
to declare your style guide, again. Which will amount to tedious duplication and loads of edge cases where the tools declare the style using different logical rules and will never match.
jscs ships with a wikimedia preset that saves lots of configuration
in
the first place.
And fortunately, jscs actually plans to ship an "autofixer" utility
that
will correct violations from within jscs itself. Using the solid
parser
and tokeniser that jscs is known for. Thus, even the small jscs rule
config
we do have, won't have to be duplicated.
The jQuery Team also supports this effort for their various
javascript
projects (both as style checker, as we do already, and as code formatter). So that's a major player we'll have on our side when it comes to continued support for the tool.
https://github.com/jscs-dev/node-jscs/issues/516
Supported by Mike Sherov (mikesherov) and Oleg Gaidarenko (markelog), from the jQuery Team (who are also jscs collaborators). And Marat Dulin (mdevils), founder of jscs.
Best,
— Krinkle
[1] https://github.com/millermedeiros/esformatter
On 20 Nov 2014, at 23:57, Derk-Jan Hartman d.j.hartman@gmail.com wrote:
Begin forwarded message:
Date: 20 november 2014 21:19:24 CET From: Jon Robson jrobson@wikimedia.org To: Bahodir Mansurov bmansurov@wikimedia.org Cc: mobile-l mobile-l@lists.wikimedia.org Subject: Re: [WikimediaMobile] Using jsbeautify in MobileFrontend
Follow up If I run `make jsbeautify` now then https://gist.github.com/jdlrobson/a05ddad00175ebceac68 is the result.
Outstanding actions:
- Need input on rest of the team about which of delete
this.cache[title]; or delete this.cache[ title ]; is the preferred standard.
- You'll notice jsbeautify and inlne comments need to be ironed out.
For example:
https://gist.github.com/jdlrobson/a05ddad00175ebceac68#file-gistfile1-diff-L...
Apart from the above 2 jsbeautify makes some adjustments to our whitspace which I guess we'll just have to live with.
Please can everyone else let me know how they think we should
proceed?
On Wed, Nov 19, 2014 at 4:12 AM, Bahodir Mansurov bmansurov@wikimedia.org wrote:
As for # Rule 4, it makes sense to add spaces inside square brackets. The reasoning is the same as why we add spaces inside parenthesis.
On Nov 18, 2014, at 12:28 PM, Jon Robson jrobson@wikimedia.org
wrote:
I explored running jsbeautify [1] on the MobileFrontend codebase and looked at how the output differs from the current styling. It introduces various rules that MobileFrontend is currently not
adhering
too. MobileFrontend already uses jscs [2] so we want to make sure the outputs of both are compatible. Here is my report on that with the recommendation that we should use it.
#Rule 1: object properties defined on a single line. e.g. { foo: 2, bar: 3 } NOT { foo: 2, bar: 3 }
I think this would be a good idea to adopt. I will explore if jscs
can
enforce this.
# Rule 2: variables that are initialised must be followed by a new line (although I noted a few odd cases e.g. in Page.js after a "?:" expression and /MobileWebClickTracking.js e.g. var Foo, bar = $( 'div' ), Baz, Bar;
not: var Foo, bar = $( 'div' ), Baz, Bar;
This will be fixed if I implement https://trello.com/c/0dkx0ldL
# Rule 3: All chained events should be indented e.g. foo() .bar();
not foo(). bar();
Seems like a no brainer. One that happens naturally most of the time.
# Rule 4: Spacing in object parameters e.g. foo[ 1 ] [ 1, 2, 3, 4 ]
not: foo[1] [1, 2, 3, 4]
This is different to MediaWiki coding conventions but I can implement https://github.com/beautify-web/js-beautify/issues/424 to give us this. We seem a bit inconsistent ourselves with this convention - let me know how you think this rule should work in our codebase...
# Rule 5: New line after variable declarations
var x, y, z;
z();
not: var x, y, z; z();
Also: function foo() {}
function bar() {}
not: function foo() {} function bar() {}
Seems like a no brainer and shouldn't introduce any major issues with code review.
# Rule 6: Comments must respect the current indentation level
if () { ... // If i is 5 we do something special } else if ( i === 5 ){
} becomes if () { ... // If i is 5 we do something special } else if ( i === 5 ){
} We'll have to think about what to do with these comments but I don't think this should be a blocker to using jsbeautify. It will only pop up occasionally.
# Rule 7: On long if statements the curly brace must be indented. And the first condition should be on the same line
if ( enableToc || ... .... && baz ) {
not: if ( enableToc || ... .... && baz ) {
Again I'm not sure if this will happen too often. This to me is a
sign
that we should be using functions rather than long if statements personally. Again not a blocker.
Actions:
- Implement https://github.com/beautify-web/js-beautify/issues/424
- You guys need to advise me on how we should handle square brackets
in our codebase in such a way we respect MediaWiki coding conventions and come up with a consistent style we are happy with and can adhere to.
- I'll implement https://trello.com/c/0dkx0ldL in some form
- I'll explore if jscs can support objects defined on new line
- When the above are done I recommend we turn on jsbeautify for the
project.
I've setup a tracking card for the above work: https://trello.com/c/5btWf2JN/90-snakes-on-a-plane and will be looking at these problems today.
[1] https://github.com/beautify-web/js-beautify [2] https://github.com/jscs-dev/node-jscs
Mobile-l mailing list Mobile-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mobile-l
Mobile-l mailing list Mobile-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mobile-l
Mobile-l mailing list Mobile-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mobile-l
Mobile-l mailing list Mobile-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mobile-l