[jitsi-dev] lib-jitsi-meet: why ESLint 'no-continue': 2?


#1

.eslintrc.js includes this rule:

  'no-continue': 2

Which means that `continue` can not be used within a loop. The
rationale given by ESlint is:

"When used incorrectly it makes code less testable, less readable and
less maintainable. Structured control flow statements such as if
should be used instead."

I cannot disagree more. I just find the usage of `continue` elegant
and much better than enclosing all the code within a if statement.

Just look at this code which makes use of a beautiful and readable `continue`:

···

-----------------------------------
    for (const c of media.candidates) {
        // Ignore RTCP candidates (we assume rtcp-mux).
        if (c.component !== 1) {
            continue;
        }

        const candidate = {
            foundation: c.foundation,
            priority: c.priority,
            ip: c.ip,
            protocol: c.transport.toLowerCase(),
            port: c.port,
            type: c.type
        };

        candidates.push(candidate);
    }
-----------------------------------

And now the same without `continue`:

------------------------------------
    for (const c of media.candidates) {
        // Ignore RTCP candidates (we assume rtcp-mux).
        if (c.component === 1) {
            const candidate = {
                foundation: c.foundation,
                priority: c.priority,
                ip: c.ip,
                protocol: c.transport.toLowerCase(),
                port: c.port,
                type: c.type
            };

            candidates.push(candidate);
        }
    }
------------------------------------

Discarding some elements is just the *exception*. I really don't want
to enclose all the useful code within a ugly "exception" if statement.
Also, imagine there are more than one exception to discard element.
Should I really place a very big and unreadable if statement plenty of
different conditions?

Could the 'no-continue': rule be set to 0 please?

--
Iñaki Baz Castillo
<ibc@aliax.net>


#2

Hello Inaki,

We had a lot of internal discussions about this rules and I don't want
to list all the argument for and against now, but if you find a code
that is really *beautiful* compared to "no continue" version feel free
to disable this rule with eslint disable rule comment.

Btw. in this example where you have only one if I like more the "no
continue" version, but it's only my personal opinion.

Regards,
Pawel

···

On Tue, Apr 25, 2017 at 9:49 AM, Iñaki Baz Castillo <ibc@aliax.net> wrote:

.eslintrc.js includes this rule:

  'no-continue': 2

Which means that `continue` can not be used within a loop. The
rationale given by ESlint is:

"When used incorrectly it makes code less testable, less readable and
less maintainable. Structured control flow statements such as if
should be used instead."

I cannot disagree more. I just find the usage of `continue` elegant
and much better than enclosing all the code within a if statement.

Just look at this code which makes use of a beautiful and readable `continue`:

-----------------------------------
    for (const c of media.candidates) {
        // Ignore RTCP candidates (we assume rtcp-mux).
        if (c.component !== 1) {
            continue;
        }

        const candidate = {
            foundation: c.foundation,
            priority: c.priority,
            ip: c.ip,
            protocol: c.transport.toLowerCase(),
            port: c.port,
            type: c.type
        };

        candidates.push(candidate);
    }
-----------------------------------

And now the same without `continue`:

------------------------------------
    for (const c of media.candidates) {
        // Ignore RTCP candidates (we assume rtcp-mux).
        if (c.component === 1) {
            const candidate = {
                foundation: c.foundation,
                priority: c.priority,
                ip: c.ip,
                protocol: c.transport.toLowerCase(),
                port: c.port,
                type: c.type
            };

            candidates.push(candidate);
        }
    }
------------------------------------

Discarding some elements is just the *exception*. I really don't want
to enclose all the useful code within a ugly "exception" if statement.
Also, imagine there are more than one exception to discard element.
Should I really place a very big and unreadable if statement plenty of
different conditions?

Could the 'no-continue': rule be set to 0 please?

--
Iñaki Baz Castillo
<ibc@aliax.net>

_______________________________________________
dev mailing list
dev@jitsi.org
Unsubscribe instructions and other list options:
http://lists.jitsi.org/mailman/listinfo/dev


#3

Thanks Pawel,

Indeed I'm gonna have more checks that may discard an element so if
you don't care I will change the ESLint rule. It can be discussed
later when a PR lands.

Best regards.

···

2017-04-25 16:58 GMT+02:00 Paweł Domas <pawel.domas@jitsi.org>:

We had a lot of internal discussions about this rules and I don't want
to list all the argument for and against now, but if you find a code
that is really *beautiful* compared to "no continue" version feel free
to disable this rule with eslint disable rule comment.

Btw. in this example where you have only one if I like more the "no
continue" version, but it's only my personal opinion.

--
Iñaki Baz Castillo
<ibc@aliax.net>


#4

Just to be clear. We don't want to have the rule changed in general,
but to disable it on case to case basis using inline comments.

···

On Tue, Apr 25, 2017 at 10:22 AM, Iñaki Baz Castillo <ibc@aliax.net> wrote:

2017-04-25 16:58 GMT+02:00 Paweł Domas <pawel.domas@jitsi.org>:

We had a lot of internal discussions about this rules and I don't want
to list all the argument for and against now, but if you find a code
that is really *beautiful* compared to "no continue" version feel free
to disable this rule with eslint disable rule comment.

Btw. in this example where you have only one if I like more the "no
continue" version, but it's only my personal opinion.

Thanks Pawel,

Indeed I'm gonna have more checks that may discard an element so if
you don't care I will change the ESLint rule. It can be discussed
later when a PR lands.

Best regards.

--
Iñaki Baz Castillo
<ibc@aliax.net>

_______________________________________________
dev mailing list
dev@jitsi.org
Unsubscribe instructions and other list options:
http://lists.jitsi.org/mailman/listinfo/dev


#5

Ah ok ok, sorry, I didn't properly understand.

I'll use inline rules then.

···

2017-04-25 17:43 GMT+02:00 Paweł Domas <pawel.domas@jitsi.org>:

Just to be clear. We don't want to have the rule changed in general,
but to disable it on case to case basis using inline comments.

--
Iñaki Baz Castillo
<ibc@aliax.net>


#6

Just to be clear. We don't want to have the rule changed in general,
but to disable it on case to case basis using inline comments.

Yep. The consensus was to have it, and disable it for a given chunk of code if it requires it. Personally, I’m fine with continue but oh well :slight_smile:

Cheers,

···

On Apr 25, 2017, at 17:43, Paweł Domas <pawel.domas@jitsi.org> wrote:

On Tue, Apr 25, 2017 at 10:22 AM, Iñaki Baz Castillo <ibc@aliax.net> wrote:

2017-04-25 16:58 GMT+02:00 Paweł Domas <pawel.domas@jitsi.org>:

We had a lot of internal discussions about this rules and I don't want
to list all the argument for and against now, but if you find a code
that is really *beautiful* compared to "no continue" version feel free
to disable this rule with eslint disable rule comment.

Btw. in this example where you have only one if I like more the "no
continue" version, but it's only my personal opinion.

Thanks Pawel,

Indeed I'm gonna have more checks that may discard an element so if
you don't care I will change the ESLint rule. It can be discussed
later when a PR lands.

Best regards.

--
Iñaki Baz Castillo
<ibc@aliax.net>

_______________________________________________
dev mailing list
dev@jitsi.org
Unsubscribe instructions and other list options:
http://lists.jitsi.org/mailman/listinfo/dev

_______________________________________________
dev mailing list
dev@jitsi.org
Unsubscribe instructions and other list options:
http://lists.jitsi.org/mailman/listinfo/dev

--
Saúl


#7

Thanks. Personally I've never seen any continue usage that makes code
worse or less readable, but the other way around. I may be wrong :slight_smile:

···

2017-04-25 17:48 GMT+02:00 Saúl Ibarra Corretgé <scorretge@atlassian.com>:

Yep. The consensus was to have it, and disable it for a given chunk of code if it requires it. Personally, I’m fine with continue but oh well :slight_smile:

--
Iñaki Baz Castillo
<ibc@aliax.net>


#8

+1 on everything Pawel said.