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

Standard JS integration #307

Closed
wants to merge 6 commits into from
Closed

Conversation

tusharmath
Copy link

Fixes : #305

@tusharmath tusharmath mentioned this pull request Apr 8, 2016
@markstos
Copy link
Collaborator

markstos commented Apr 8, 2016

@tusharmath this pull request doesn't pass our Travis tests because there are hundreds of StandardJS failures.

@tusharmath
Copy link
Author

@markstos That's the intent. You know when to merge, when all the issues are fixed.

@tusharmath
Copy link
Author

Total Errors: 1935

@tusharmath
Copy link
Author

Total Errors: 232

@tusharmath
Copy link
Author

@markstos Take a look at the errors. Apart from style errors, it has also highlighted errors such as — using undeclared variables , using == instead of ===. This could lead to potential issues in production.

@tusharmath tusharmath changed the title chore(travis): integrate standardjs with travis Standard JS integration Apr 8, 2016
@tusharmath
Copy link
Author

@markstos There is one particular failure —

/node-config/lib/config.js
  247:40  error  'fnName' is not defined

I am not sure how to fix it. May be you can help me here?

@lorenwest
Copy link
Collaborator

That's a straight-up bug. Instead of

...config.' + fnName + '()...

It should be

...config.watch()...

It looks like that code was copied from a more generic place into the watch function, without making it specific to watch().

@lorenwest
Copy link
Collaborator

This PR has too many changes. Is there a way to change a configuration such that the errors are significantly reduced to match the coding style of the package, and the only errors are real improvements like == to === and undeclared variables?

@tusharmath
Copy link
Author

@lorenwest No we can't configure it. Check the FAQ.

If the changes are too big to review, I would recommend you go commit by commit. That way u will be able to review all the changes.

@tusharmath
Copy link
Author

@lorenwest Some weird issue on node 0.8 can you try re-running after clearing cache on travis?

@tusharmath
Copy link
Author

Looks like the issue was with snazzy module which doesn't work with version 0.8 because of its dependency on inherits module. I have removed it for now as it just a prettfier for standardjs's output. Once the issue is fixed, I guess we can re-integrate it later.

PS: Build is fixed!

@lorenwest
Copy link
Collaborator

This was a large amount of work and as much as I dislike the size of the PR, the point is to endure some pain in order to achieve consistency.

My concern is that manual changes are intermixed with automated changes, making it impractical to detect the changes due to reformatting vs. the changes due to manual code. Normally that'd be mitigated by tests, but the tests were changed along with the code.

My suggestion to getting this merged is to split the work into multiple pull requests:

  1. All manual changes needed
  2. Automated changes to lib/config.js alone, showing existing tests pass
  3. Automated changes to the rest of the package, including tests
  4. Travis hooks so earlier commits don't break travis

I sincerely appreciate the work you've put into this and the consistency it brings to the library. If you can break it into these multiple pull requests, I will take the time necessary to get them committed and pushed to NPM once they're all in.

-Loren

@tusharmath
Copy link
Author

tusharmath commented Apr 16, 2016

Closing this in favour of #310

@tusharmath tusharmath closed this Apr 16, 2016
@markstos markstos mentioned this pull request Aug 25, 2016
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.

None yet

3 participants