j-maly / CommandLineParser

Command line parser. Declarative arguments support. Rich set of argument types (switches, enums, files, etc...). Mutually exclusive arguments validations.
MIT License
137 stars 30 forks source link

#55 Added StrongDefaultValue for ValueArgument #57

Closed eapyl closed 5 years ago

eapyl commented 6 years ago

As it is described in #55

twpol commented 5 years ago

I think this change means that many (all?) existing uses of DefaultValue now have to use ForcedDefaultValue, which caused me quite a lot of confusion when updating the version of CommandLineParser just now. I can raise an issue with full details and code if you need it.

StefH commented 5 years ago

Sorry for the confusion. Please raise an issue with the usage differences for before/after this code.

alexandre-lecoq commented 5 years ago

I have the same issue. However, my understanding is that it's not really a bug, but rather an unconscious and unadvertised breaking change. Also could not find anything about ForceDefaultValue in the documentation.

Basically, i had this code :

        [ValueArgument(
            typeof(int),
            "connectiontimeout",
            DefaultValue = 5,
            Description = "The time to wait for a connection to open.",
            FullDescription = "The time (in seconds) to wait for a connection to open. The default value is 5 seconds.",
            Example = "15")]
        public int ConnectionTimeout { get; set; }

This code would behave like this :

Now with the introduction of ForceDefaultValue the code behaves like :

This is how it broke my application. My understanding, is that there is a change of semantic of DefaultValue. It was the default value for both the lack of passed parameter and the lack of value for a passed parameter. It seems to me, now, those 2 cases are split :

Therefore if I understand correcly, in the latest version, I to change my code to :

        [ValueArgument(
            typeof(int),
            "connectiontimeout",
            DefaultValue = 5,
            ForcedDefaultValue = 5,
            Description = "The time to wait for a connection to open.",
            FullDescription = "The time (in seconds) to wait for a connection to open. The default value is 5 seconds.",
            Example = "15")]
        public int ConnectionTimeout { get; set; }

That is, for ValueArgument, if I want to keep the old behavior, wherever I have DefaultValue, I now have to also add ForcedDefaultValue.

Can you confirm ?

I'm not raising an issue because I think there's no (simple) going back except reverting the new feature. It would have been nice to document the change and change the major version number of the package though.

StefH commented 5 years ago

Thank you for this information. I will try to understand and investigate the code and see if I can bring the old behaviour back.

alexandre-lecoq commented 5 years ago

I think the cause is here : https://github.com/j-maly/CommandLineParser/blob/40120ac02a670dae0ace4e21103d97be370ad46e/src/CommandLineArgumentsParser/Arguments/ValueArgument.cs#L428

alexandre-lecoq commented 5 years ago

I proposed a fix in PR #68