Skip to content
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: prefer-t-throws #156

Open
gajus opened this issue Nov 13, 2016 · 10 comments · May be fixed by #345
Open

Rule proposal: prefer-t-throws #156

gajus opened this issue Nov 13, 2016 · 10 comments · May be fixed by #345
Labels
💵 Funded on Issuehunt This issue has been funded on Issuehunt enhancement help wanted

Comments

@gajus
Copy link
Contributor

gajus commented Nov 13, 2016

Issuehunt badges

I was reviewing some codes using ava and found this pattern repeating often:

try {
  await request(requestOptions);
} catch (error) {
  t.true(error.statusCode === 500);
}

This can be written instead as:

const error = await t.throws(request(requestOptions));

t.true(error.statusCode === 500);

I think the latter should be preferred.


IssueHunt Summary

Backers (Total: $80.00)

Submitted pull Requests


Become a backer now!

Or submit a pull request to get the deposits!

Tips

@jfmengels
Copy link
Contributor

Sounds like a good idea to me 👍. Not sure when/why try/catch makes sense in tests when t.throws is available.

The second example is better, but FYI, using t.fail() would improve your first example.

try {
  await request(requestOptions);
  t.fail();
} catch (error) {
  t.true(error.statusCode === 500);
}

@gajus
Copy link
Contributor Author

gajus commented Nov 13, 2016

The second example is better, but FYI, using t.fail() would improve your first example.

Another idea for a lint rule. :-)

@sindresorhus sindresorhus changed the title rule proposal: no try-catch Rule proposal: no-try-catch Nov 14, 2016
@sindresorhus
Copy link
Member

sindresorhus commented Nov 14, 2016

👍 Sounds good. PR welcome :)

I think prefer-t-throws would be a better name. We don't want to prevent all usage of try/catch. Thoughts?

@jfmengels
Copy link
Contributor

Agreed

@sindresorhus sindresorhus changed the title Rule proposal: no-try-catch Rule proposal: prefer-t-throws Nov 14, 2016
@gajus
Copy link
Contributor Author

gajus commented Nov 14, 2016

I think prefer-t-throws would be a better name. We don't want to prevent all usage of try/catch. Thoughts?

What would be a valid use case for try..catch that cannot translate to t.throws?

@jfmengels
Copy link
Contributor

jfmengels commented Nov 14, 2016

I think that t.throws() handles every case you'd use a try catch for, and even the Promise#catch() case.

@sindresorhus
Copy link
Member

@gajus Maybe something that might throw, but that you don't care about in the test, so you'd like to silence it. I'm sure there are other cases we can't think of too. But the main point is that the intent is clearer with prefer-t-throws than no-try-catch; We want to recommend using t.throws(), not arbitrarily prevent try/catch.

@vadimdemedes
Copy link

The immediate valid use case for try..catch that came up in my mind is error handling in Koa:

app.use(function * (next) {
  try {
    yield* next;
  } catch (err) {
    // handle error
  }  
});

@vadimdemedes
Copy link

We don't want to prevent all usage of try/catch.

Definitely agree with this.

@IssueHuntBot
Copy link

@IssueHunt has funded $80.00 to this issue.


@issuehunt-oss issuehunt-oss bot added the 💵 Funded on Issuehunt This issue has been funded on Issuehunt label May 10, 2019
Mesteery added a commit to Mesteery/eslint-plugin-ava that referenced this issue Feb 6, 2022
@Mesteery Mesteery linked a pull request Feb 6, 2022 that will close this issue
Mesteery added a commit to Mesteery/eslint-plugin-ava that referenced this issue Feb 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💵 Funded on Issuehunt This issue has been funded on Issuehunt enhancement help wanted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants