Document and finalize standardized code formatting

[sashadev-sky]

as a follow up to #378, I wanted to confirm some straggling syntaxes still not sure about, and also just document our code syntax in a wiki maybe (eventually we can make a contributing.md)

So far the standard we will implement is:

• 2 space indent
• single quotes
• space in blocks, no space in objects

Yep. Makes sense. You see, { return; } qualifies as code, or more specifically a block of code that runs (executes one line per session by the intepreter), whereas { deselect: true } is not the same but an object, and is interpreted in one go, unlike some block of code, hence the rules for both differ.

• column limit: 80 recommended. No column limit on comments but be reasonable
• No space before parentheses: function() {}
• no comma after var declaration style
• trailing commas for objects
• camelCase variables

Pending:

• comment syntax

[sashadev-sky]

@rexagod I was curious about:

    if (!this._moved) {
      return;
    }

Sometimes we will see the above syntax, sometimes we will see:

    if (!this._moved) { return; }

Is there a linter we can add that keeps it on the same line unless the line goes over length limit, and in that case goes to a new line? Is this something we want? Also what is the current line length limit that is set?

[sashadev-sky]

Ok I added 3 configs:

        "curly": ["error", "multi-line"],
        "brace-style": ["error", "1tbs", { "allowSingleLine": true }],
        "block-spacing": ["error", "always"],

that basically allow if (!this._moved) { return; } in terms of having the return on the same line and spaces in between the braces and the return.

It does not allow spaces in between objects still: so map.fire('singleclick', {deselect: true}); is ok but map.fire('singleclick', { deselect: true }); is not.

let me know what you think!

[rexagod]

Yep. Makes sense. You see, { return; } qualifies as code, or more specifically a block of code that runs (executes one line per session by the intepreter), whereas { deselect: true } is not the same but an object, and is interpreted in one go, unlike some block of code, hence the rules for both differ.

Maybe https://eslint.org/docs/rules/object-curly-spacing could help here?

[sashadev-sky]

what are your thoughts on the var? (last point). Google says no to the one declaration followed by comma syntax. I kind of like how it looks compared to doing something like what we currently have:

rotateBy: function(angle) {
    var map = this._map;
    var center = map.project(this.getCenter());
    var corners = {0: '', 1: '', 2: '', 3: ''};
    var i;
    var p;
    var q;

    for (i = 0; i < 4; i++) {
      p = map.project(this.getCorner(i)).subtract(center);
      q = L.point(
          Math.cos(angle) * p.x - Math.sin(angle) * p.y,
          Math.sin(angle) * p.x + Math.cos(angle) * p.y
      );
      corners[i] = map.unproject(q.add(center));
    }

    this.setCorners(corners);

    // window.angle = L.TrigUtil.radiansToDegrees(angle);

    this.rotation -= L.TrigUtil.radiansToDegrees(angle);

    return this;
  },

I thought also it was pretty standard to do that in ES6, like with object destructuring too for ex. honestly just curious

[sashadev-sky]

@rexagod prepared to get this serious about our style guide?

This Style Guide uses RFC 2119 terminology when using the phrases must, must not, should, should not, and may. The terms prefer and avoid correspond to should and should not, respectively. Imperative and declarative statements are prescriptive and correspond to must.

[rexagod]

Haha. I am, but the question is, will the first timers be okay with this standard? Also, I agree with the no comma after var declaration style, looks more cleaner to me. So +1 for that!

[sashadev-sky]

@rexagod the block of vars look cleaner to you you’re saying?

I felt the opposite, but I can see your perspective too and functionally it is actually cleaner. So +1 from me too

[sashadev-sky]

@rexagod up next: trailing commas? We don't do that at all, but I'm kinda with it. Makes sense functionally. Thoughts?

[sashadev-sky]

@jywarren looping in

[rexagod]

Trailing commas? Sorry, can you provide an example of those?

[sashadev-sky]

like commas on the last key value pairs in objects and after the last method in the class. Even though nothing follows it. Makes it easier to add new methods

[rexagod]

Oh. Those! Yeah they definitely do, all good from me!

[sashadev-sky]

@rexagod ok then:
Leaflet always puts a space before parentheses in function declarations like function () { }

Do you have any opinions on this one? I kinda think function() { } is more standard right?

[rexagod]

I'm glad to see that we're working on such minute levels, kudos to your research! Hmm. I guess `function() {}` is standard, but since this is a Leaflet library after all, maybe `function () {}` should be our choice? I'm +1 with whatever decision you make here. Pranshu Srivastava | @rexagod

…

On Sat, 24 Aug 2019, 09:42 Sasha Boginsky, ***@***.***> wrote: ok last one (for now): Leaflet always puts a space before parentheses in function declarations like function () { } Do you have any opinions on this one? I kinda think function() { } is more standard right? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#389?email_source=notifications&email_token=AIAAUZ6RL27E6T76ZTS7HPLQGCYKFA5CNFSM4INK7QN2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5BYE5Y#issuecomment-524518007>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AIAAUZ6CR3GYDA3NXZ5LG53QGCYKFANCNFSM4INK7QNQ> .

[jywarren]

Just a note on "function() {}" vs "function () {}" -- actually "function someDescriptiveName() {}" is really helpful for debugging as it shows up better in the console, which would also answer the question of whether the space is required (in that case, yes!). What do you think? On Sat, Aug 24, 2019 at 1:13 AM Pranshu Srivastava <[email protected]> wrote:

…

I'm glad to see that we're working on such minute levels, kudos to your research! Hmm. I guess `function() {}` is standard, but since this is a Leaflet library after all, maybe `function () {}` should be our choice? I'm +1 with whatever decision you make here. Pranshu Srivastava | @rexagod On Sat, 24 Aug 2019, 09:42 Sasha Boginsky, ***@***.***> wrote: > ok last one (for now): > Leaflet always puts a space before parentheses in function declarations > like function () { } > > Do you have any opinions on this one? I kinda think function() { } is > more standard right? > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > < #389?email_source=notifications&email_token=AIAAUZ6RL27E6T76ZTS7HPLQGCYKFA5CNFSM4INK7QN2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5BYE5Y#issuecomment-524518007 >, > or mute the thread > < https://github.com/notifications/unsubscribe-auth/AIAAUZ6CR3GYDA3NXZ5LG53QGCYKFANCNFSM4INK7QNQ > > . > — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#389?email_source=notifications&email_token=AAAF6J42STHULIZDWNSFPSDQGC7OBA5CNFSM4INK7QN2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5BY5OQ#issuecomment-524521146>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAF6JYD5A77MTAROATTMOTQGC7OBANCNFSM4INK7QNQ> .

[sashadev-sky]

@jywarren I think you wrote the function without a space and said thats better for debugging but then said space should be required? Sorry can you clarify

[jywarren]

Oh no, sorry i mean having the function named instead of anonymous is better for debugging. But adding a function name then means you must have a space -- like, function myFunctionName(). Oh, i see -- yeah, well, no space between the function name and the (). But yes between the word function and myFunctionName. Sorry, i didn't think of that way of reading it! Thank you!

…

On Tue, Aug 27, 2019 at 7:53 PM Sasha Boginsky ***@***.***> wrote: @jywarren <https://github.com/jywarren> I think you wrote the function without a space and said thats better for debugging but then said space should be required? Sorry can you clarify — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#389?email_source=notifications&email_token=AAAF6J7SLRCMP66VMV6TZXTQGW44PA5CNFSM4INK7QN2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5JOOPA#issuecomment-525526844>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAF6J64KNFZAGXR5MIORRDQGW44PANCNFSM4INK7QNQ> .

[rexagod]

I'm all +1 for named functions as well, they are way more distinguishable in the error stack trace, hence better debugging!

[rexagod]

Okay so it seems we'll be going ahead with no spaces, right?

[sashadev-sky]

@rexagod yes!

[sashadev-sky]

@rexagod @jywarren ok i think this is last! Comments -
should we say

// this one is for a one-liner on its own line

return exampleCode; /* this one is for a one-liner that is inline with the code */

/**
 * this is a multi-line comment block
 * it is integrated with JSDoc - JSDoc comment snippet is suggested after typing `/**`
 * guessing its personal config but if we start using JSDoc later, can suggest ppl integrate that
 */

// this is fine too
// for multi-line comments

I don't mind left behind TODO comments. CodeCov threw an error on my PR in MK for this yesterday so just saying..

I set Max Length for comments to be ignored: "max-len": ["warn", { "ignoreComments": true }]
Agree / Disagree? Should we compromise for instead a more lenient max length for comments or does this just really not matter? I just don't like the way comments will span over more lines and take up more space following the current max length of 80.

For JSDoc commenting, for now maybe lets hold off on that until we get our code standard and then reopen the issue later as part of a 'standardizing documentation' discussion?

• "valid-jsdoc": 2 is in .eslintrc.js but has been deprecated as of ESLint v5.10.0 (we are on 6.1.0) so either way lets change to "valid-jsdoc": 0?
• getting errors for eslint(require-jsdoc) even though its also deprecated and not even included under rules in .eslintrc.js. So let's add `"require-jsdoc": 0?
• eslint-plugin-jsdoc suggested by eslint instead in deprecation notice. we can install when decide we want to start jsdocing?