-
Notifications
You must be signed in to change notification settings - Fork 18
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
Added error logging #4
base: master
Are you sure you want to change the base?
Conversation
@marcello3d - I could do with this functionality being on npm. This PR from @mtscout6 is more comprehensive than my code, so I'd appreciate the three of us working on it and getting it published. @mtscout6 - There is one thing I did differently in my implementation that I will raise in a second. I hope you don't mind -- this is a useful learning process for me. |
Cursorily, this looks good, but the style change (adding semi-colons), makes the diff difficult to read. If it isn't too much trouble, could you match the style of the existing code? |
I added semi-colons and a gulp task to lint the code. Not using the semi-colons is a bad practice. See JavaScript the Good Parts by Douglas Crockford. |
Linting is great, but I disagree that skipping semi-colons is bad practice or introduces bugs. I've chosen this style because I find it clearer and more consistent. Two good posts on the topic: http://blog.izs.me/post/2353458699/an-open-letter-to-javascript-leaders-regarding and http://inimino.org/~inimino/blog/javascript_semicolons |
Do with this as you will, I'm not a fan of your decision. You are welcome to cherry-pick my changes or ignore this pull request. In all I'm unhappy with the state of this codebase. There were no tests until I added a very small subset of some. That coupled with a less then common style paradigm from that of the majority of the community. |
Without listening to errors you would have no idea why the watch isn't working when errors exist.