Skip to content
This repository has been archived by the owner on Mar 7, 2021. It is now read-only.

JSHint cleanup #125

Closed
wants to merge 5 commits into from
Closed

JSHint cleanup #125

wants to merge 5 commits into from

Conversation

XhmikosR
Copy link
Contributor

BTW @cgiffard, I could set up JSCS and or ESLint to make things more consistent across the codebase.

Also, I was thinking, maybe we should just move JSHint in package.json script section?

@cgiffard
Copy link
Member

Yep. The test encapsulation of JSHint dates to a time where JSHint had a really rubbish CLI. Now, it's fine. :)

@XhmikosR
Copy link
Contributor Author

All right, I'll update the PR and ping you.

@XhmikosR
Copy link
Contributor Author

@cgiffard: updated. Let me know if you need something else. https://github.com/cgiffard/node-simplecrawler/pull/125/files?w=1

PS.

BTW @cgiffard, I could set up JSCS and or ESLint to make things more consistent across the codebase.

@cgiffard
Copy link
Member

There are a couple of tiny stylistic things that I wanna tweak with the array literal syntax, but other than that, looks good! I'm really tired so I'll do a manual merge in the morning. :)

@cgiffard
Copy link
Member

I've used JSCS and it'd be cool to get it integrated to ensure contributors' style is correct. I might have a crack at it tomorrow (unless you get to it first!)

@XhmikosR
Copy link
Contributor Author

There are a couple of tiny stylistic things that I wanna tweak with the array literal syntax, but other than that, looks good! I'm really tired so I'll do a manual merge in the morning. :)

Do you mean in this patch or in general?

I've used JSCS and it'd be cool to get it integrated to ensure contributors' style is correct. I might have a crack at it tomorrow (unless you get to it first!)

Should be easy to add it so I might have some time to make a PR later, after this is merged.

@XhmikosR
Copy link
Contributor Author

Allright, I'm done for now. I used the JSHint rules you see in this PR and ESLint. Would be nice to have this automated but I'll leave it up to you after this is merged.

@@ -34,8 +42,9 @@ crawler.emit = function(name, queueItem) {
return string;
}

if (!~boringEvents.indexOf(name))
if (!~boringEvents.indexOf(name)) {

Choose a reason for hiding this comment

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

Unexpected use of '~'.

@XhmikosR
Copy link
Contributor Author

@cgiffard: please step up and make that @houndci guy go away from this PR. Not only he is wrong, he keeps spamming the PR with useless comments.

Otherwise I will just close this and be done with it.

@XhmikosR
Copy link
Contributor Author

XhmikosR commented May 5, 2015

@cgiffard: bump for feedback.

Also, clean up JSHint options and make them stricter.
Used JSHint and ESLint; fixed all JSHint issues, most ESLint ones.
[ci skip]
@cgiffard
Copy link
Member

Cleaning up old PRs — going to do a bit of a code cleanup soon, so I figured I'd close this one for now.

@cgiffard cgiffard closed this Sep 10, 2015
@XhmikosR
Copy link
Contributor Author

I could rebase if needed since I've done the work already.
On Sep 10, 2015 13:37, "Christopher Giffard" notifications@github.com
wrote:

Closed #125 #125.


Reply to this email directly or view it on GitHub
#125 (comment).

@cgiffard
Copy link
Member

Sure, I did a quick review and it looks good. If you're happy to rebase, I'll merge!

@cgiffard
Copy link
Member

There's definitely style stuff there that could evolve, but my feelings on this have mellowed — I think it's more important to be consistent than right.

@XhmikosR
Copy link
Contributor Author

Rebased this but apparently you have closed it.

@cgiffard
Copy link
Member

I tried to reopen, but it looks like you might need to file a new PR... it won't let me. :-/

Sorry, mate.

@XhmikosR
Copy link
Contributor Author

Ah, OK, no worries. Opened #163 .

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants