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

Nicer defaults #48

Closed
wants to merge 2 commits into from
Closed

Nicer defaults #48

wants to merge 2 commits into from

Conversation

devinrhode2
Copy link

Added colors.js for nicer console messages

Turned debugging off by default, can be turned back on with --debug
Made --no-restart-on exit default, can still pass in --no-restart-on error, or --keep-restarting to tell the server to continue to restart on exit.

Changed the default watch to the program.js file passed in, instead of the current directory. Perhaps we also want to include /node_modules, we'll find out either way.

These are fixes to my frustrations.


} else if (arg === "--keep-restarting" || arg === '-k') {
noRestartOn = null;
} else if (arg === "--no-restart-on-error" || arg === '-k') {
Copy link
Author

Choose a reason for hiding this comment

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

Accidentally carried over || arg === '-k', this should be deleted.

@iangreenleaf
Copy link
Collaborator

If it's not too much trouble, could you separate these into different pull requests? That way we can consider/discuss each of the ideas one at a time. Looks like you're new to git - you can use git add -p to stage only certain lines from a bunch of changes. git commit turns those staged changes into a commit, rinse and repeat.

I can tell you right now that I'll be hesitant to alter the --no-restart behavior. It would be a surprising change for anyone who's depending on supervisor to keep a process running despite occasional crashes.

And I almost certainly won't change the default --watch behavior. Most node apps split logic up amongst multiple files which are require'd by the main file. So if we aren't watching all of those, supervisor isn't much use.

@devinrhode2
Copy link
Author

I was also thinking about publishing this more basic version as a different npm, seems like that'd be easiest, no?

@iangreenleaf
Copy link
Collaborator

I'm thinking that a solution to some of these problems may be to allow some sort of .supervisorconfig file, where users can store their preferred config options and have them automatically picked up by supervisor. Judging by what issues get raised, it seems like people use supervisor in 3 or 4 distinct ways, and I'm not sure that one set of defaults is ever going to satisfy everyone.

@devinrhode2
Copy link
Author

Hey did you ever do the .supervisorconfig thing? I actually just got this same idea for another cli tool.

@iangreenleaf
Copy link
Collaborator

I haven't gotten around to it, but it still sounds like a good idea to me. I'd take a pull request, or else I may get to it eventually.

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

2 participants