Add/extra globals #125

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
2 participants
@rauchg
Contributor

rauchg commented Aug 24, 2012

Added a new option so that you can whitelist arbitrary globals from jshintrc

@brentlintner

This comment has been minimized.

Show comment Hide comment
@brentlintner

brentlintner Aug 27, 2012

Contributor

Thanks for the contribution. This looks good to me (in general), aside from some failing tests (but I need to fix others that are failing, so I can fix them myself if need be). I do have one nitpick question, though: Would you be open to calling the option globals instead of extraglobals? It feels slightly more succinct (at least, IMO), and I don't think that would cause conflicts. Additionally, it might also ensure a (relatively) easier upgrade path for the implementation of what is suggested in https://github.com/jshint/node-jshint/issues/104 (or just allow for the issue to be closed).

Thoughts?

Contributor

brentlintner commented Aug 27, 2012

Thanks for the contribution. This looks good to me (in general), aside from some failing tests (but I need to fix others that are failing, so I can fix them myself if need be). I do have one nitpick question, though: Would you be open to calling the option globals instead of extraglobals? It feels slightly more succinct (at least, IMO), and I don't think that would cause conflicts. Additionally, it might also ensure a (relatively) easier upgrade path for the implementation of what is suggested in https://github.com/jshint/node-jshint/issues/104 (or just allow for the issue to be closed).

Thoughts?

@rauchg

This comment has been minimized.

Show comment Hide comment
@rauchg

rauchg Aug 27, 2012

Contributor

@brentlintner naming is up to you - tests were passing when I tried, maybe I forgot to include some commits. The reason I didn't go with globals is that maybe the implication there is that you override the other globals? Which is obviously not the case.

Contributor

rauchg commented Aug 27, 2012

@brentlintner naming is up to you - tests were passing when I tried, maybe I forgot to include some commits. The reason I didn't go with globals is that maybe the implication there is that you override the other globals? Which is obviously not the case.

@brentlintner

This comment has been minimized.

Show comment Hide comment
@brentlintner

brentlintner Aug 27, 2012

Contributor

@guille Ah, OK. I can re-commit your commits with the option renamed to globals and merge into master, then (unless you want to do it, and be the committer). No worries about the tests (however odd)- I think it is a quick fix, anyways. I'll post when I've published, as well.

Contributor

brentlintner commented Aug 27, 2012

@guille Ah, OK. I can re-commit your commits with the option renamed to globals and merge into master, then (unless you want to do it, and be the committer). No worries about the tests (however odd)- I think it is a quick fix, anyways. I'll post when I've published, as well.

@rauchg

This comment has been minimized.

Show comment Hide comment
@rauchg

rauchg Aug 27, 2012

Contributor

Go ahead. Thanks for considering this @brentlintner

Contributor

rauchg commented Aug 27, 2012

Go ahead. Thanks for considering this @brentlintner

@rauchg rauchg closed this Aug 27, 2012

@brentlintner

This comment has been minimized.

Show comment Hide comment
@brentlintner

brentlintner Aug 30, 2012

Contributor

Sorry, this took a bit longer than I intended (work got in the way). Published all this as v0.8.2. If you are interested, check out master for your commits! :-)

Thanks again,

Contributor

brentlintner commented Aug 30, 2012

Sorry, this took a bit longer than I intended (work got in the way). Published all this as v0.8.2. If you are interested, check out master for your commits! :-)

Thanks again,

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