Hey, James. Congrats on the project and thanks for the effort. You'll see I proposed a couple of PR's tonight, with bug fixes that I didn't think you'd feel needed any discussion.
But now I have another suggestion and before implementing the change (which I'm happy to do), I wanted to run it by you first.
People who just download the repo and try to run it will fail for a reason you don't warn about in the Readme: you hard-code a path to a Windows folder for the jvm. If people don't have it there already/also, the Commandbox server fails to start.
This could be easily solved by using a config property that just lets Commandbox pull down the jvm and use it. I'd think that should be the default approach, with a comment on how one could instead change it to use an external jvm, as you have it set for.
Would you agree? I've already coded it on my end and could have just pushed a pr, but I thought it a significant enough change to ask first. It will also help others to see how you regard (and how you may prefer handling) such suggestions.
Some maintainers love help, others let PR's languish; some are very guarded about what changes they'll accept, and some will even make the change themselves and close the PR without accepting it (which happened to me last week in another repo, which was dismaying and of course will discourage contributions to that repo--unrelated to cfml).
I suspect you'll be open to at least considering suggestions, and I look forward to proposing still more if so. Of course, I realize you may well not AGREE with every proposed change. :-)
Hey, James. Congrats on the project and thanks for the effort. You'll see I proposed a couple of PR's tonight, with bug fixes that I didn't think you'd feel needed any discussion.
But now I have another suggestion and before implementing the change (which I'm happy to do), I wanted to run it by you first.
People who just download the repo and try to run it will fail for a reason you don't warn about in the Readme: you hard-code a path to a Windows folder for the jvm. If people don't have it there already/also, the Commandbox server fails to start.
This could be easily solved by using a config property that just lets Commandbox pull down the jvm and use it. I'd think that should be the default approach, with a comment on how one could instead change it to use an external jvm, as you have it set for.
Would you agree? I've already coded it on my end and could have just pushed a pr, but I thought it a significant enough change to ask first. It will also help others to see how you regard (and how you may prefer handling) such suggestions.
Some maintainers love help, others let PR's languish; some are very guarded about what changes they'll accept, and some will even make the change themselves and close the PR without accepting it (which happened to me last week in another repo, which was dismaying and of course will discourage contributions to that repo--unrelated to cfml).
I suspect you'll be open to at least considering suggestions, and I look forward to proposing still more if so. Of course, I realize you may well not AGREE with every proposed change. :-)