--stopOnUpdateErrors behaves inconsitently

[ttaomae]

Based on the README I would expect that simply specifying --stopOnUpdateErrors would enable this feature and produce <stopOnUpdateErrors>true</stopOnUpdateErrors> in the app.xml manifest. Based on the behavior of other boolean options, I would expect that I need to specify either --stopOnUpdateErrors=true or --stopOnUpdateErrors=false to enable or disable it, respectively. However, the actual behavior is that specifying no value does not enable the feature and specifying any value, including false or an empty value, will enable the feature (e.g. --stopOnUpdateErrors=anything, --stopOnUpdateErrors=false, and --stopOnUpdateErrors= all produce <stopOnUpdateErrors>true/<stopOnUpdateErrors> in app.xml).

In addition, this option will be included in the parameters because it is not skipped here: https://github.com/edvin/fxlauncher/blob/9537c329a5bd42191f69055b9dbe9805b210887b/src/main/java/fxlauncher/CreateManifest.java#L81-L90

And lastly, this option is named differently than all others, which are named-like-this.

[ttaomae]

I can submit a pull request for this, but I'd like to confirm that this is in fact behaving incorrectly and to verify what the desired behavior is before I start working on it.

My preference would be to make it consistent with the other boolean parameters, which means:

• Rename to stop-on-update-errors.
• Require a value of true or false.
• Do not include in parameters.

However, I understand if something closer to the documented behavior is desired.

[edvin]

You're right, this should be made consistent. I think we should deprecate the old keyword and introduce the new. Also, it should not be included in parameters.

A PR would be most welcome. Sorry for not replying to this issue earlier, I've been insanely busy as of late :)

[ttaomae]

When you say "deprecate" do you mean that the old keyword should still exist alongside the new keyword or should it be removed?

[edvin]

Yes, it should exist, but emit a warning. Then we'll remove it completely in the coming release.

[ttaomae]

Should the current (incorrect) behavior of the old keyword remain the same or should it also be fixed to behave as documented?

[edvin]

It should be fixed, it's clearly a bug if it doesn't have as documented IMO :)

[ttaomae]

Sure, that makes perfect sense. I just wasn't sure if you considered backwards compatibility to be an issue in this case. For example, it is possible that someone is relying on the current behavior, even if it is not behaving as documented.

I think that's all the questions I have for now. I'll begin working on this when I get a chance, but that probably won't be until some time next week.

[edvin]

I think if you're relying on behavior caused by a bug, that user has to fix his code, even if we caused him to do it this way. It's a bit unfortunate, but not unexpected.

Great, really looking forward to this fix :)