Skip to content

Simplifying tests #111

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

Merged
merged 1 commit into from
May 19, 2016
Merged

Simplifying tests #111

merged 1 commit into from
May 19, 2016

Conversation

jfmengels
Copy link
Contributor

@jfmengels jfmengels commented May 18, 2016

Fixes #72
WIP, just tackled one of the rules, to see the reactions. Does this still look good to you @sindresorhus @jamestalmage @twada? If so, I'll go and do the others.

@jamestalmage
Copy link
Contributor

Yes. This is fantastic.

@sindresorhus
Copy link
Member

👌

@jfmengels
Copy link
Contributor Author

There you go. I think we can improve it a bit further by handling cases like isCalleeMemberExpression or hasTestModifier, but this is at least a good start and handles the most common case.

return currentTestNode;
};

rule.if = rest(function (predicates) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

[nits] if is a reserved word for ECMAScript so we better avoid it.

Good alternatives? when ?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's only needed for ES3 backwards compatibility.

https://mathiasbynens.be/notes/javascript-properties

Copy link
Collaborator

@twada twada May 19, 2016

Choose a reason for hiding this comment

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

Yep, so it's just a nitpicking.
But it also makes syntax highlighting annoying IMO.

Copy link
Contributor Author

@jfmengels jfmengels May 19, 2016

Choose a reason for hiding this comment

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

Yeah, colorwise, it gets shown with the same color as an if in my editor, and I thought that was actually pretty nice, as that highlights pretty well what it does. Happy to change it to when if you want.

Copy link
Member

@sindresorhus sindresorhus May 19, 2016

Choose a reason for hiding this comment

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

I prefer if. We don't care about ES3. If your editor is highlighting it incorrectly, I suggest opening and issue/PR on your editor.

Sublime is very open to pull requests for syntax highlighting now btw: https://github.com/sublimehq/Packages

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In Atom for instance, the return method of Bluebird

return Promise.resolve(foo)
  .return(bar);

also gets highlighted like the return keyword. Which is nice IMO :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed to go with if 👍

@twada
Copy link
Collaborator

twada commented May 19, 2016

LGTM 👍

@jamestalmage jamestalmage merged commit 9ce3807 into master May 19, 2016
@jamestalmage jamestalmage deleted the simplify-tests branch May 19, 2016 14:19
@jfmengels
Copy link
Contributor Author

season-4-the-simpsons-4x2-l2JdVRfJozpjq70SA

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

Successfully merging this pull request may close these issues.

4 participants