Skip to content

Added error logging#4

Open
mtscout6 wants to merge 3 commits into
marcello3d:masterfrom
mtscout6:error-logging
Open

Added error logging#4
mtscout6 wants to merge 3 commits into
marcello3d:masterfrom
mtscout6:error-logging

Conversation

@mtscout6
Copy link
Copy Markdown
Contributor

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

@davegurnell
Copy link
Copy Markdown

@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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