Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

test:lint Lint added for test folder and also fixed intial level of lint errors #25280 #25288

Closed
wants to merge 1 commit into from

Conversation

saquibkhan
Copy link

test:lint Lint added for test folder and also fixed intial level of lint errors

Added closure linter for test folder. Fixed 2k+ issues in 300 files.

Fixes #25280

@saquibkhan
Copy link
Author

@misterdjules thanks for the comment. I have updated the code accordingly.

@misterdjules
Copy link

@joyent/node-coreteam Now that I'm thinking about it, I would actually advocate for landing that in v0.12. If we land this in master, it's going to be painful when we merge changes in tests from v0.12 to master because the two will have diverged significantly. It would probably not require any change to #25280 since master and v0.12 are probably identical currently.

@saquibkhan
Copy link
Author

@misterdjules Done!

@misterdjules
Copy link

@saquibkhan Thanks, let's wait for @joyent/node-coreteam to give us some feedback on my proposal for landing this in v0.12 instead of master, and then we'll do a thorough code review (although at first sight changes look good).

Thank you once again! 👍

@cjihrig
Copy link

cjihrig commented May 13, 2015

@misterdjules yes, 0.12 makes more sense since there should be no behavior change.

@saquibkhan saquibkhan force-pushed the fix-issue-25280 branch 2 times, most recently from 5f99d62 to d2671a4 Compare May 13, 2015 17:46
Added closure linter for test folder. Fixed 2k+ issues in 300 files.
After this fix there is no lint error in test scripts.

Fixes nodejs#25280
@jasnell
Copy link
Member

jasnell commented Aug 15, 2015

@misterdjules @cjihrig ... what would you like to do with this one?

@cjihrig
Copy link

cjihrig commented Aug 15, 2015

The converged repo uses ESLint. I don't personally see a need for this, but others might.

@jasnell
Copy link
Member

jasnell commented Aug 15, 2015

Hmm.. I'm inclined to close. If someone wanted to do this in v0.12 a new PR can be opened

@jasnell
Copy link
Member

jasnell commented Aug 17, 2015

Closing this one here. Can revisit if necessary but it's not likely to land.

@jasnell jasnell closed this Aug 17, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants