New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rule Proposal: `arrow-block-style` #4109

Closed
mysticatea opened this Issue Oct 10, 2015 · 12 comments

Comments

Projects
None yet
5 participants
@mysticatea
Member

mysticatea commented Oct 10, 2015

This rule is a styling rule for "non block" v.s. "single return block".

There is a trap in arrow function that returns an object.

let foo = () => {};

This {} is a block, not an empty object.
This should be replaced with:

let foo = () => ({});
// or
let foo = () => { return {}; };
// or
let foo = () => undefined;
// or
let foo = () => { /* do nothing */ };

This rule has an option.

  • "as-needed" (default) - If it's possible, this suggests a use of expression without a block.
  • "always" - This enforces using a block.

{
    "arrow-block-style": [2, "as-needed"]
}

Valid

// OK, this is not a block.
let foo = () => 0;

// OK, this cannot replace with non-block (without a comma-sequence).
let foo = (retv, name) => {
    retv[name] = true;
    return retv;
};

// OK, this is considered as having some side effects, so the block is proper.
let foo = () => { bar(); };

// OK, it's explicit functions that does nothing.
// This behavior is the same as http://eslint.org/docs/rules/no-empty
let foo = () => { /* do nothing */ };
let foo = () => {
    // do nothing.
};

Invalid

// This can be replaced with `() => 0` simply.
let foo = () => {
    return 0;
};

// This `{}` is a block.
// it should be fixed to `() => ({})` or `() => undefined` or `() => { /* do nothing */ }`.
let foo = () => {};

{
    "arrow-block-style": [2, "always"]
}

Valid

let foo = () => {
    return 0;
};
let foo = (retv, name) => {
    retv[name] = true;
    return retv;
};

Invalid

let foo = () => 0;

@alberto

This comment has been minimized.

Show comment
Hide comment
@alberto

alberto Oct 13, 2015

Member

@mysticatea are you working on this?

Member

alberto commented Oct 13, 2015

@mysticatea are you working on this?

@BenoitZugmeyer

This comment has been minimized.

Show comment
Hide comment
@BenoitZugmeyer

BenoitZugmeyer Oct 13, 2015

Contributor

Awesome. I would love to see a third option, as-needed-line, in the same spirit of curly multi-line, enforcing one-liners to behave like as-needed and multi-line functions like always.

{
    "arrow-block-style": [2, "as-needed-line"]
}
Valid
// OK, one liner, valid
let foo = () => 0;

// OK, cannot be replaced by non-block
let foo = (retv, name) => {
    retv[name] = true;
    return retv;
};

// OK, this is considered as having some side effects, so the block is proper.
let foo = () => { bar(); };

// OK, multiline
let foo = () => {
    return 0;
};
Invalid
// Invalid, multiline
let foo = () =>
    0;

// Invalid, multiline
let foo = () => foo(
    true,
    false);

Should I open a new issue for this?

Contributor

BenoitZugmeyer commented Oct 13, 2015

Awesome. I would love to see a third option, as-needed-line, in the same spirit of curly multi-line, enforcing one-liners to behave like as-needed and multi-line functions like always.

{
    "arrow-block-style": [2, "as-needed-line"]
}
Valid
// OK, one liner, valid
let foo = () => 0;

// OK, cannot be replaced by non-block
let foo = (retv, name) => {
    retv[name] = true;
    return retv;
};

// OK, this is considered as having some side effects, so the block is proper.
let foo = () => { bar(); };

// OK, multiline
let foo = () => {
    return 0;
};
Invalid
// Invalid, multiline
let foo = () =>
    0;

// Invalid, multiline
let foo = () => foo(
    true,
    false);

Should I open a new issue for this?

@mysticatea

This comment has been minimized.

Show comment
Hide comment
@mysticatea

mysticatea Oct 14, 2015

Member

@alberto I'm not working on this :)

@BenoitZugmeyer I'd like to separate issue. But the rule does not exist yet. Umm...

Member

mysticatea commented Oct 14, 2015

@alberto I'm not working on this :)

@BenoitZugmeyer I'd like to separate issue. But the rule does not exist yet. Umm...

@BenoitZugmeyer

This comment has been minimized.

Show comment
Hide comment
@BenoitZugmeyer

BenoitZugmeyer Oct 14, 2015

Contributor

OK no problem, I'll open an issue when this rule is implemented.

Contributor

BenoitZugmeyer commented Oct 14, 2015

OK no problem, I'll open an issue when this rule is implemented.

@alberto

This comment has been minimized.

Show comment
Hide comment
@alberto

alberto Oct 14, 2015

Member

Ok, I have the examples mentioned here covered, but I'm not sure all potential cases are.
For example, this is also probably an error:

var foo = () => {bar: 1};

Should this be an error too?

var foo = () => { a };
Member

alberto commented Oct 14, 2015

Ok, I have the examples mentioned here covered, but I'm not sure all potential cases are.
For example, this is also probably an error:

var foo = () => {bar: 1};

Should this be an error too?

var foo = () => { a };
@mysticatea

This comment has been minimized.

Show comment
Hide comment
@mysticatea

mysticatea Oct 14, 2015

Member

Wow, thank you all.

@alberto Good point. I am not sure this rule should catch those pattern since those will be caught by other rules (e.g. no-unused-expression). Well, is there a rule which allows only loop in LabeledStatement? ({foo: bar()} is caught by other?) I want to hear advices. @eslint/eslint-team

Member

mysticatea commented Oct 14, 2015

Wow, thank you all.

@alberto Good point. I am not sure this rule should catch those pattern since those will be caught by other rules (e.g. no-unused-expression). Well, is there a rule which allows only loop in LabeledStatement? ({foo: bar()} is caught by other?) I want to hear advices. @eslint/eslint-team

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Oct 15, 2015

Member

Right now there's http://eslint.org/docs/rules/no-labels, no-label-var, no-empty-label. I don't think there's a rule that only allows labels in a loop

Member

hzoo commented Oct 15, 2015

Right now there's http://eslint.org/docs/rules/no-labels, no-label-var, no-empty-label. I don't think there's a rule that only allows labels in a loop

@mysticatea

This comment has been minimized.

Show comment
Hide comment
@mysticatea

mysticatea Oct 15, 2015

Member

@hzoo Thank you!

no-empty-label seems to catch () => {foo: bar()}, so in my opinion, special handling is not needed for those two pattern. I guess those are handled in the same path as let foo = () => { bar(); }; (the block does not have one ReturnStatement).
But my opinion is not strong.

Member

mysticatea commented Oct 15, 2015

@hzoo Thank you!

no-empty-label seems to catch () => {foo: bar()}, so in my opinion, special handling is not needed for those two pattern. I guess those are handled in the same path as let foo = () => { bar(); }; (the block does not have one ReturnStatement).
But my opinion is not strong.

@alberto

This comment has been minimized.

Show comment
Hide comment
@alberto

alberto Oct 15, 2015

Member

In which category should this rule fall under?

Member

alberto commented Oct 15, 2015

In which category should this rule fall under?

@alberto

This comment has been minimized.

Show comment
Hide comment
@alberto

alberto Oct 15, 2015

Member

Nevermind, I just saw there is a specific category for ES6 features.

Member

alberto commented Oct 15, 2015

Nevermind, I just saw there is a specific category for ES6 features.

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Oct 15, 2015

Member

Yeah, I don't think we need this to catch any further patterns, as we're covered by other rules.

Member

nzakas commented Oct 15, 2015

Yeah, I don't think we need this to catch any further patterns, as we're covered by other rules.

@alberto

This comment has been minimized.

Show comment
Hide comment
@alberto

alberto Oct 18, 2015

Member

Can anyone have a look at this?

Member

alberto commented Oct 18, 2015

Can anyone have a look at this?

@alberto alberto closed this in c97c4c0 Oct 26, 2015

ilyavolodin added a commit that referenced this issue Oct 26, 2015

Merge pull request #4144 from alberto/issue4109
New: arrow-body-style rule (fixes #4109)

lizardruss added a commit to codeschool/eslint that referenced this issue Oct 29, 2015

@eslint eslint bot locked and limited conversation to collaborators Feb 7, 2018

@eslint eslint bot added the archived due to age label Feb 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.