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

Added error logging #4

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

mtscout6
Copy link
Contributor

Without listening to errors you would have no idea why the watch isn't working when errors exist.

@davegurnell
Copy link

@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.

@marcello3d
Copy link
Owner

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?

@mtscout6
Copy link
Contributor Author

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.

@marcello3d
Copy link
Owner

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

@mtscout6
Copy link
Contributor Author

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.

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.

3 participants