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

modify AnyFunction to include generator functions #131

Merged
merged 1 commit into from
Apr 6, 2017
Merged

modify AnyFunction to include generator functions #131

merged 1 commit into from
Apr 6, 2017

Conversation

arsaniwilliam
Copy link
Contributor

Using typeofEq('function') for the AnyFunction to include generator functions.
Note: The test uses a generator function (function*(x){... ) but that causes the es3 linter to report a parse error on the *.. Is there any way to avoid this?

@davidchambers
Copy link
Member

The fix for the parsing error may be to remove --env es3 from the value assigned to ESLINT in the makefile, and to specify $(ESLINT) --env es6 ... -- test. Give that a go. :)

@arsaniwilliam
Copy link
Contributor Author

@davidchambers that worked. thank you!

Makefile Outdated
@@ -35,6 +35,7 @@ lint:
-- index.js
$(ESLINT) \
--env node \
--env es6 \
--env mocha \
Copy link
Member

Choose a reason for hiding this comment

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

The order in which --env values are provided isn't significant, but I like --env es6 --env node --env mocha as language-version > engine > framework seems like the appropriate hierarchy. I love to avoid making arbitrary decisions. :)

Also, please add --env es3 to the $(ESLINT) ... -- index.js command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

any preference to where the --env es3 should go in the -- index.js command? :)

Copy link
Member

Choose a reason for hiding this comment

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

Let's put it first there too. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's what I thought :)

@davidchambers
Copy link
Member

This looks great, Arsani!

As a last step please squash the commits and --force push:

$ git rebase 829cece41561c879b2ff040d1ed269f75ba72e2c^ --interactive
$ git push origin AnyFunction-IncludeGeneratorType --force

While you're at it, I suggest shortening the commit message to match the pull request title:

-modify the AnyFunction to include generator functions, by using typeofEq('function').
+modify AnyFunction to include generator functions

It's advisable to avoid lines longer than 72 characters in a commit message. :)

@arsaniwilliam
Copy link
Contributor Author

done!

@davidchambers
Copy link
Member

Very nice!

The commit is not being mapped to your GitHub account. If this bothers you, you can fix the issue by telling GitHub about the email address you included in the commit, or by amending the commit to use the same email address you use on GitHub. If this doesn't bother you, I'll go ahead and merge.

@arsaniwilliam
Copy link
Contributor Author

I made my email public on github.. are there any more steps I need to take?

@arsaniwilliam
Copy link
Contributor Author

fixed the author and email on the commit..

@arsaniwilliam
Copy link
Contributor Author

@davidchambers Thanks again for all your help.
Feel free to merge if you'd like :)

Makefile Outdated
@@ -27,13 +27,15 @@ README.md: index.js
.PHONY: lint
lint:
$(ESLINT) \
--env es6 \
Copy link
Member

Choose a reason for hiding this comment

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

I just noticed that this is es6 rather than es3. Let's keep it as es3 to ensure that we don't accidentally sneak ES6 features into the source code (it's okay to use ES6 features in the test suite).

Please update the commit with git commit --amend. You'll then need to use --force when you push to your branch.

@arsaniwilliam
Copy link
Contributor Author

whoops... good catch.
fixed.

@davidchambers davidchambers merged commit bb0e3fd into sanctuary-js:master Apr 6, 2017
@arsaniwilliam arsaniwilliam deleted the AnyFunction-IncludeGeneratorType branch April 7, 2017 00:05
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.

None yet

2 participants