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 <[email protected]> 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 <[email protected]> > 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 <[email protected]> > 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 <[email protected]> > 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 <[email protected]> > >> > wrote: > >> > > >> > Begin forwarded message: > >> > > >> > Date: 20 november 2014 21:19:24 CET > >> > From: Jon Robson <[email protected]> > >> > To: Bahodir Mansurov <[email protected]> > >> > Cc: mobile-l <[email protected]> > >> > 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-L257 > >> > > >> > 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 > >> > <[email protected]> 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 <[email protected]> > 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 > >> > [email protected] > >> > https://lists.wikimedia.org/mailman/listinfo/mobile-l > >> > > >> > > >> > > >> > _______________________________________________ > >> > Mobile-l mailing list > >> > [email protected] > >> > https://lists.wikimedia.org/mailman/listinfo/mobile-l > >> > > >> > > >> > > >> > >> _______________________________________________ > >> Mobile-l mailing list > >> [email protected] > >> https://lists.wikimedia.org/mailman/listinfo/mobile-l > > > > >
_______________________________________________ Mobile-l mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mobile-l
