-
Notifications
You must be signed in to change notification settings - Fork 63
Fix for #620 and #224: prevent implicit conversion of nil to string #647
Conversation
I am actually thinking, why not just throw an error if ml.server isn't properly configured? Not sure if that will have side-effects on commands that don't need an env, but if it doesn't it would give proper feedback about properties being mal-configured.. |
I'd still keep the second change by the way, just change the first into an exception.. |
Pushed in a commit with your suggestion @grtjn |
Don't always blindly believe my suggestions.. ;-) We should run some tests with this. I recall some feature in latest slush-marklogic-node depends on ./ml dev info returning results without dev.properties. Also not sure what happens with commands that don't take env (like ml upgrade and such).. |
Took me a couple days but I was able to try out every command and couldn't find any impact to the upgrade command, the other general commands or the scaffolding commands. The bootstrapping commands and deployment/data commands app all appeared to be positively impacted. I learned that the settings command and some others threw nasty exceptions that were difficult to interpret as missing *.properties files without it. Example of settings error without the new exception:
I will investigate the impact on Slush soon. |
Looks good to me. @grtjn you happy with this? |
@grtjn I did a review of Slush and can't find any code to suggest it would be impacted by this. I tried it out on a Slush install I already had up and running and it seemed ok. If Slush is impacted by this for some reason I'd personally prefer that Slush finds another way to accomplish whatever it needs than by relying on returning non-existent properties.. |
This looks good to me. |
I implemented two fixes:
In my testing, either fix appeared to independently solve #620 and #224