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

else statement returns still emits a return a value error #8

Closed
barkthins opened this Issue Apr 8, 2016 · 7 comments

Comments

Projects
None yet
3 participants
@barkthins
Copy link

barkthins commented Apr 8, 2016

The error each then() should return a value is reported even though the .then always returns a promise in the code below.

Apparently the logic for following return values can't evaluate an else clause

    return oracle.getPrices(c)
    .then(function(p) {
        if (p.price != 0) {
            return p.price.toNumber() / 1e6;
        } else {
            // XXX unavoidable race
            return 200;
        }
    });  

@barkthins barkthins changed the title else statement returns still get return a value error else statement returns still emits a return a value error Apr 8, 2016

@xjamundx

This comment has been minimized.

Copy link
Owner

xjamundx commented Apr 8, 2016

right because I personally prefer the no-else-return rule, so in my case I'd still do the return 200 at the root level of the function.

Yeah totally valid bug though, solution is tricky to enforce properly.

We could check that there's any return inside of a then function. For example:

.then(function(p) {
    if (p < 10) {
        return p * 1000;
    }
})

But then it obviously misses cases like this where the root of the function isn't covered, but to some extent that is covered by consistent-return.

What do you think? Would you be able to try to make the code change?

@barkthins

This comment has been minimized.

Copy link

barkthins commented Apr 8, 2016

funny thing is worked on medical devices and prefer else-return. Please cite why no-else-return is a rule...

If you don't have enough of the parse tree to figure out all the cases possible and make sure there's a return for each case, then you probably can't solve the problem. I don't know enough about how eslint works to know whether that's possible.

@xjamundx

This comment has been minimized.

Copy link
Owner

xjamundx commented Apr 8, 2016

@barkthins I use no-else-return in javascript, because of the endless callback functions. This saves one level of nesting on the else path

As far as creating custom ESLint rules, we do have a full tree, it's just the tools for traversing it are pretty weak, so checking every path is pretty difficult. Code path analysis landed which is supposed to make this easier and may be worth checking out.

I'd love your help adding this ability to the rule, let me know if you can do it!

@barkthins

This comment has been minimized.

Copy link

barkthins commented Apr 8, 2016

@xjamundx I use promises. What's this issue with callback nesting you speak of? ;-P

My plate is stacked high and deep, being at a startup. I'm busy enough answering Ethereum questions, I'm afraid another community will give me overload...

@xjamundx

This comment has been minimized.

Copy link
Owner

xjamundx commented Apr 8, 2016

K I'll keep this open until I get some time to figure it out!

@scottnonnenberg

This comment has been minimized.

Copy link

scottnonnenberg commented May 12, 2016

Not exactly the same, but very similar - I get an 'each then() should return a value' error for this:

.then(([response, text]) => {
  try {
    return [response, parseWithDates(text)];
  }
  catch (e) {
    return [response];
  }
})

As well as for this, from my tests:

return fetch.default()
  .then(() => {
    throw new Error('expected error to be thrown');
  })
  .catch(error => {
    expect(error).to.have.property('message', 'Something went wrong!');
  });
@xjamundx

This comment has been minimized.

Copy link
Owner

xjamundx commented May 12, 2016

ahh thanks for the note @scottnonnenberg I'll try to look at this..in the meantime you may be abe to do something like this:

var dates
try { dates = parseWithDates(text) }
catch (e) { data = undefined }
return [response, dates]

ahuff44 added a commit to ahuff44/eslint-plugin-promise that referenced this issue Oct 7, 2016

add code path analysis to always-return
This will also change the reported location of errors to the inner
function that failed to return, rather than referring to it's enclosing
Promise

fixes xjamundx#8
fixes xjamundx#18
fixes xjamundx#24

ahuff44 added a commit to ahuff44/eslint-plugin-promise that referenced this issue Oct 7, 2016

add code path analysis to always-return
This will also change the reported location of errors to the inner
function that failed to return, rather than referring to it's enclosing
Promise

I bumped up the the version number (2.0.1 -> 3.0.0) because the changed
location reporting could un-suppress linter errors that have been
suppressed with `// eslint-disable-line promise/always-return`

fixes xjamundx#8
fixes xjamundx#18
fixes xjamundx#24

@xjamundx xjamundx closed this in #25 Oct 7, 2016

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