Skip to content
This repository was archived by the owner on Feb 25, 2020. It is now read-only.

Conversation

@ptmt
Copy link

@ptmt ptmt commented Aug 16, 2014

This is first commit in this pull request, nothing important.

The background. I would be happy to use both type of configuration: via command-line or via hash parameter. I use the code below in the commit to change port inside my environment where 8080 port is already taken with other application.

To conitinue I have few questions:

  1. Could you provide a test for run.js which has as much command-line arguments as you can do specify?
  2. Could you please add jshint and/or jscs configuration files for Javascript styleguides. I use Atom right now, but almost every popular text editor / IDE can follow these guidelines and makes collaborate much easier.
  3. Could you please add TravicCI or CodeShip integration, so every commit in my pull requests will be tested and linted and you will be able to review code faster and with a bit smaller chance of mistake.

@olegp
Copy link
Owner

olegp commented Aug 16, 2014

Comments from @alexlamsl:

  • extend() is unused
  • port and cluster vars being declared seems a bit verbose, these can just be inlined
  • var options redeclaring an argument is just asking for trouble, should be options = options || {}instead of var options = options || {}

Please fix.

@ptmt
Copy link
Author

ptmt commented Aug 16, 2014

Sure, looks reasonable, and jshint has warnings about this. As I said it 'not-for-merge' commit, mostly for demonstration what problem is there for me. Thanks for review, looking forward to contribute more quality code after some experiments.

@ptmt
Copy link
Author

ptmt commented Sep 25, 2014

updated

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.

2 participants