common-node icon indicating copy to clipboard operation
common-node copied to clipboard

More flexible and explicit configuration of common-node application

Open ptmt opened this issue 11 years ago • 3 comments

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.

ptmt avatar Aug 16 '14 06:08 ptmt

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.

olegp avatar Aug 16 '14 15:08 olegp

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 avatar Aug 16 '14 15:08 ptmt

updated

ptmt avatar Sep 25 '14 13:09 ptmt