Code style listing dependencies (AMD)

[fredck]

There are two ways to list dependencies that we can take in considerations:

One line:

CKE.define( [ 'ckeditor', 'tools', 'mvc/model' ], function( CKE, tools, Model ) {} );

Multi-line 1:

CKE.define( [
    'ckeditor',
    'tools',
    'mvc/model'
], function(
    CKE,
    tools,
    Model
) {} );

Multi-line 2:

CKE.define( [
    'ckeditor',
    'tools',
    'mvc/model'
], function( CKE, tools, Model ) {} );

We have these possibilities:

1. Always "One line".
2. Always "Multi-line".
3. "Multi-line" only if the line gets longer than a limit.
4. "Multi-line" only if a minimum number of requires is reached.

[mlewand]

I just can't help the feeling that multi line approach is ultra crappy, but it's the only one to work I guess : D

Multi line:

• Takes a lot of space (LOC). It's especially bad because it's just in a beginning of file...
• Totally wastes horizontal space.
• Looks just stupid...
• + More diff friendly.

Single line:

• When there's more than 6 modules it's just pure hell. That would result with Maximum line-length #19 violation at day one...
• Any updates are harder than in multi line approach
• + Fits more into our general codestyle.
• + When keept small looks perfectly.

I've read @fredck proposition in other topic:

One line up to a number of dependencies (5). I like this one.

But it makes just another IF, and when it comes down to standard we should limit IFs (doubts) as much as possible.

Reading this proposition I've got an idea to do up to five per line, like so:

define( [
    'foo', 'bar', 'baz', 'bom', 'events',
    'gui', 'backendBus', 'reporter', 'serializer',
    'stringUtils'
], function(
    foo, bar, baz, bom, events,
    gui, backendBus, reporter, serializer,
    stringUtils
) {

} );

It makes it easy to count, still somewhat maintainable, but I have a feeling that in practice it would result with problems combined from both multi line and single line appraoch : D And still it's a sample of iffing standard.

So at the end I consider only two options: single line and multi line.

Because all of that, my choice would go for multi line... though I don't like it : \ and it feels bad.

[jodator]

Keep in mind that you can have long dependency names and short list of parameters.

I've end up with something like this for most of modules:

define( [
    'jquery',
    'backbone',
    'My/Super/Some/Module/Foo',
    'My/Super/Some/Module/Bar',
], function( jQuery, Backbone, Foo, Bar ) {
    // code here
} );

The longer is the dependency list the more something wrong here warning should be raised.

• I only have one module that have ultra long dependency list which is very ugly but it ties all components and is only one.
• I'm breaking dependency list only if it is longer then line max.
• I usually don't have long list of dependencies, but If I do I put each dependency path in its own line

I'd also would go with multiline if long because of readability and diffs. Also I think that you will have ad-banner blidness after some time and it wouldn't be noticable since you will work inside the file.

[Reinmar]

The problem with options 3. and 4. is that when adding one dependency which exceeds the limit you may be forced to reformat the entire list and that's not cool.

Therefore, even though multiline-always looks kind of silly I would go with it, because it has other advantages like diffing, fast to add, fast to remove, no thinking needed. Although, if there's only one dependency, then we should use the short version.

The good middle ground, though, is what @jodator proposed and I'm totally fine with it. Module paths are usually much longer than the variable names which we want to expose, so this solution is reasonable.

@mlewand:

Reading this proposition I've got an idea to do up to five per line, like so:

This requires reorganising half of the list if you want to add a dependency in the middle of it. That's a lot of work and terrible diffs.

[fredck]

Note that we'll be mostly requiring modules provided by core, so their "paths" tend to be short. But ofc we'll always have the longer paths cases.

[fredck]

I don't think that avoiding switching single to multiple line because of diffs is a strong enough argument. This is one part of the code that is pretty easy to understand, so even if we have bigger diffs on that, it'll be pretty easy to understand what's going on. So this should not be a blocker.

[fredck]

I've added @jodator proposal to the description, so we can take it in consideration. It looks good for a few dependencies, but it may become problematic once you have lots of them.

When it comes to multi-line, I'm still for option "1", which is btw the common practice out there.

[jodator]

@fredck I would go with: "One line" but if long do "Multi-line 2". Also lots of dependencies shouldn't occur much or at all.

[oleq]

It's all about #19. Clarity first, rules next. I'm for 4. with limit=1.

CKE.define( [ 'ckeditor' ], function( CKE, tools, Model ) {} );

but

CKE.define( [
    'ckeditor',
    'tools'
], function( CKE, tools, Model ) {} );

It's diff-friendly and still keeps us within line-length limitation.

[fredck]

I'm for "4" and "Multi-line 2" with limit=3.

[oleq]

@fredck Why not limit=2 or 4? What's behind your decision?

[fredck]

@fredck Why not limit=2 or 4? What's behind your decision?

1. Reduce the occurrence of multi-line.
2. Code may still be readable without hurting the eyes.

Ofc, this is just IMHO. Others may disagree and we'll reach a consensus (never! :D)

[adelura]

If we'll have one deps per line it will be easier to our brain to catch them, but if we want to have more on one line don't let out brain think too much so intead this:

define( [
    'foooooo', 'bar', 'baz', 'bom', 'events',
    'gui', 'backendBus', 'reporter', 'serializer',
    'stringUtils'
] );

do this:

define( [
    'foooooo',     'bar',        'baz',           'bom',
    'events',      'gui',        'backendBus',    'reporter',
    'serializer',  'stringUtils'
] );

Also lots of dependencies shouldn't occur much or at all.
If we'll end up with lot's of dependencies it's quite possible that we are doing something wrong :D

[fredck]

@adelura we're not taking in consideration having more than one dependency per line in multi-line. The possible patterns are those in the issue description.

[fredck]

Current proposal: 4 with limit 1 or 3 (to decide). Let's close this.

[jodator]

I would go with 3 with limit "longer then 120 chars"

Rationale: It's dependent on dependency name/path length. Simple and consitent for me.

One file can have simple dependencies which looks ok in one line:

define( [ 'lodash', 'jquery', 'foo', 'bar' ], function( _, jQuery, foo, bar ){
    // code here
} );

while other file may break with just 2:

define( [
    'Utilities/Data/Processors/Markdown/FooProcessor',
    'Utilities/Data/Processors/Markdown/BarProcessor',
], function( Foo, Bar ) {
    // code here
} );

If not go with limit 2 (just because of above case)

[fredck]

Most of the CKEditor deps will come from core, so their paths will be short. I've just put the following in 120 loc:

define( [ 'lodash', 'jquery', 'foo', 'bar', 'jquery', 'foo', 'bar' ], function( _, jQuery, foo, ba, jQuery, foo, bar ){
    // code here
} );

So still the limit will be beneficial:

define( [
    'lodash',
    'jquery',
    'foo',
    'bar',
    'jquery',
    'foo',
    'bar'
], function( _, jQuery, foo, ba, jQuery, foo, bar ){
    // code here
} );

In any case the 120 chars limit apply, so your second example will in any case be the right shot then.

[jodator]

Most of the CKEditor deps will come from core, so their paths will be short. I've just put the following in 120 loc:

If you assume flat directory structure (or put everything in one folder) ;-) Anyway I don't know if there will be such structure or not. But when taking advantage of splitting current plugin.js to multiple files there might be some long paths.

@fredck I'm not sure on what we agree/disagree here (3 or 4?) I'm on 3. Your examples are for 3?

[fredck]

@jodator I'm for "4", with limit=3. Still "3" partially applies, because it must in any case conform with the general 120 chars limit.

[jodator]

@fredck then ok

[Reinmar]

This got totally outdated - we're using ES6 modules now so there's no doubt how to format imports.