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

detect improper use of t.throws #742

Merged
merged 8 commits into from
Apr 11, 2016

Conversation

jamestalmage
Copy link
Contributor

Protects against a common misuse of t.throws (Like that seen in #739).

This required the creation of a custom babel plugin.

https://github.com/jamestalmage/babel-plugin-ava-throws-helper

Sample output:

Improper usage of t.throws detected at /Users/jamestalmage/dev/ava/test/fixture/improper-t-throws.js (4:10).

You should wrap the following expression in a function:

  throwSync()

Like this:

  function() {
    throwSync()
  }

See https://github.com/sindresorhus/ava#throwsfunctionpromise-error-message for more details.

The linked documentation isn't that helpful. We should probably make a short recipe about this. (maybe a common-mistakes recipe? This seems like too small a topic for a whole recipe). We can update the URL when that recipe is written.

Protects against a common misuse of t.throws (Like that seen in avajs#739).

This required the creation of a custom babel plugin.

https://github.com/jamestalmage/babel-plugin-ava-throws-helper
@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @spudly, @ariporad and @ingro to be potential reviewers

'You should wrap the following expression in a function:',
' %s',
'Like this:',
' function() {\n %s\n }',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Promote arrow functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered that.

But if the user is green enough that that they don't fully understand how throw works, I was concerned about piling on another abstraction they need to understand.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea fair enough.

@novemberborn
Copy link
Member

The linked documentation isn't that helpful. We should probably make a short recipe about this

👍

(maybe a common-mistakes recipe?

#404?

@vadimdemedes
Copy link
Contributor

/Users/jamestalmage/dev/ava/test/fixture/improper-t-throws.js (4:10)

Perhaps displaying relative path would be better and would align with our "clean stack traces" feature.

@jamestalmage jamestalmage force-pushed the use-ava-throws-helper branch from 9a6d76a to dbed3d4 Compare April 10, 2016 20:43
@jamestalmage
Copy link
Contributor Author

Updated with relative file paths and colors:

screenshot 2016-04-10 16 44 14

@vadimdemedes
Copy link
Contributor

I know it is an overkill, but would be incredibly cool to have syntax highlight in errors (not just this one).

@jamestalmage
Copy link
Contributor Author

I know it is an overkill, but would be incredibly cool to have syntax highlight in errors (not just this one).

Like this?:

screenshot 2016-04-11 01 49 24

@jamestalmage
Copy link
Contributor Author

I think maybe the middle section:

The following expression:
  t.throws()

Is kinda useless with the codeframe right above.

@jamestalmage
Copy link
Contributor Author

screenshot 2016-04-11 02 07 31

@vadimdemedes
Copy link
Contributor

Super awesome!

@sindresorhus
Copy link
Member

Marvelous. :shipit:

@novemberborn
Copy link
Member

Cool!

Should update the default plugin sections in the readme and babelrc recipe.

@jamestalmage
Copy link
Contributor Author

Should update the default plugin sections in the readme and babelrc recipe.

I went with just the babelrc recipe. I feel like most user's don't care, so I don't want to pollute the readme with every side-effect free plugin we install. Really, we could just update the readme to say. "AVA always includes a few internal plugins regardless of configuration, but they should not impact the behavior of hour code", and then link to the Notes section of the babelrc recipe.

@sindresorhus
Copy link
Member

"AVA always includes a few internal plugins regardless of configuration, but they should not impact the behavior of hour code", and then link to the Notes section of the babelrc recipe.

👍

@jamestalmage jamestalmage merged commit 3201b1b into avajs:master Apr 11, 2016
@jamestalmage jamestalmage deleted the use-ava-throws-helper branch April 11, 2016 10:45
@jfmengels
Copy link
Contributor

🎉

@sindresorhus
Copy link
Member

🍾✨

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants