Introduce Thor2, a class with more intuitive defaults

[MaxLap]

In order to make things more intuitive for new users without breaking backward compatibility, I propose adding a new base class for CLIs. I had no idea how to call that class, so I just went with Thor2, feel free to change it.

Here are the more expected changed you get from inheriting from Thor2:

• check_unknown_options!
• check_default_type!
• exit_on_failure? returning true

Once this class is in the code, documentation could be changed to tell users to start from Thor2. From there on, some additional doc could be added to explain some of the options that have barely any documentation to customize either Thor or Thor2.

I think this is better than having the doc recommend to use X and Y options to everyone.

[MaxLap]

Alright, fixed the failures. Care to take a look @rafaelfranca? I think it would be great if this was in the next release.

Also, if there are other defaults you think would make sense to be changed, now would be the time.

[marcandre]

Seems like more sensible defaults.

How about Thor::CLI?

[rafaelfranca]

I can see argument to change those defaults but I don't think it is worth the trouble to add a new base class that will just create confusing to people neither is worth a breaking release.

Maybe a better path would ask people to explicitly set what the behavior they want in their commands sowing a deprecation in case the behavior is undefined and in a future breaking release we can flip the default since all existing code is explicit about which behavior they want.

So for existent code either check_unknown_options! or allow_unknown_options! should be called or a deprecation will be show. Same for the other options.

[MaxLap]

Thank you for the feedback @rafaelfranca, but I must disagree. (I hope the following doesn't sound harsh, there is a lot to say so I try to keep things short)

Downsides of your suggestion:

• forcing people to copy paste code for new and old CLI from the documentation
• deprecation warning displayed to people who often don't have control on it. (Users of gems that use thor)
• a future breaking change that will for sure catch people off-guard when they make a big jump that bypasses the deprecation window (it always happens)
• longer before all of this can be considered "done"
• More doc changes. Since the doc will need to be changed multiple times, it will most likely require more changes overall.

Downside of my suggestion:

• Some confusion for users between 2 classes to use

In both cases, users should specify a version requirement for their thor dependency. Either because of the new class being used, or because of the new class method used to avoid the deprecation warning. (Actually it will require that twice with your proposition, one for the class_method, and one after the breaking change)

@marcandre Thor::CLI could also make sense, but I feel it is more confusing as this doesn't show any sense of progression compared to Thor, someone could think one is an alias for the other.

An option, instead of Thor2, could be to do something similar to Rails' migration versioning. Something like Thor[2] or Thor[2018] or Thor[0.21]. So using it is class MyCLI < Thor[2].

[rafaelfranca]

I'm aware of the downside but I really don't want to maintain two base classes. Either we move everyone to the new defaults slowly or we keep the defaults as is.

There is no explanation why this new class exists other than "We can't change the default so now we introduced two defaults".

Thor is still in a pre 1.0 release so another option is change the default in thor 1.0 and have people deal with the breakage to upgrade to 1.0.

[Choms]

I agree with @rafaelfranca and IMHO the best option would be to keep the current defaults for compatibility, and allow to override the defaults for those using either ENV vars (my preferred option) or a config file. Using the env vars approach would actually be a two line change:

      def check_unknown_options #:nodoc:
        @check_unknown_options = ENV["THOR_CHECK_UNKNOWN_OPTIONS"] =~ /true/i ? true : nil
        @check_unknown_options ||= from_superclass(:check_unknown_options, false)
      end

[marcandre]

For deprecation warnings, a good approach could be to issue those only for particular uses that break the new defaults. For example check_default_type is really just bullet proofing, most dependents of thor probably don't rely on it. All default types could be checked, and a deprecation warning would be issued only if a check_default_type: true would raise an error in a future version. For most cases though, changing the default would have no effect and no deprecation warning need to be issued.

check_unknown_options is the same: for most cases there wouldn't need be a deprecation warning as the most users wouldn't actually need to allow_unknown_options.

Same idea for exit_on_failure?, only if an exception is rescued and exit_on_failure? is not redefined should a warning be issued.

I think the @chroms idea to use the ENV variables would be better suited to suppress the deprecation warnings than to affect the behavior. This gives the power to the users of the packages affected to get on with their lives while the maintainers of the affected packages straighten things up.

[marcandre]

Example diff:

diff --git a/lib/thor/base.rb b/lib/thor/base.rb
index c5f004e..f86afe5 100644
--- a/lib/thor/base.rb
+++ b/lib/thor/base.rb
@@ -159,7 +159,7 @@ class Thor
       end
 
       def check_default_type #:nodoc:
-        @check_default_type ||= from_superclass(:check_default_type, false)
+        @check_default_type ||= from_superclass(:check_default_type, nil)
       end
 
       def check_default_type? #:nodoc:
diff --git a/lib/thor/parser/option.rb b/lib/thor/parser/option.rb
index f434b47..d68d607 100644
--- a/lib/thor/parser/option.rb
+++ b/lib/thor/parser/option.rb
@@ -111,7 +111,7 @@ class Thor
 
     def validate!
       raise ArgumentError, "An option cannot be boolean and required." if boolean? && required?
-      validate_default_type! if @check_default_type
+      validate_default_type!
     end
 
     def validate_default_type!
@@ -127,8 +127,15 @@ class Thor
       when Hash, Array, String
         @default.class.name.downcase.to_sym
       end
-
-      raise ArgumentError, "Expected #{@type} default value for '#{switch_name}'; got #{@default.inspect} (#{default_type})" unless default_type == @type
+      if default_type != @type
+        err = "Expected #{@type} default value for '#{switch_name}'; got #{@default.inspect} (#{default_type})"
+        if @check_default_type
+          raise ArgumentError, err
+        elsif @check_default_type == nil
+          warn "Deprecation warning: #{err}." +
+            'This will be rejected in the future unless you explicitly pass the options `check_default_type: false`.'
+        end
+      end
     end
 
     def dasherized?

[marcandre]

Realized in the 🚿 that I needed fixing my example diff to show we'd set the default to nil and distinguish it from an explicit false 😅

[Choms]

@marcandre's idea makes sense to me :)

…

On Sep 21, 2018 8:39 PM, "Marc-André Lafortune" ***@***.***> wrote: Realized in the 🚿 that I needed fixing my example diff to show we'd set the default to nil and distinguish it from an explicit false 😅 — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#621 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB0ffFudnsfaClxAVzR3oyP9MC2Jlxkeks5udTJhgaJpZM4WqdmJ> .

[rafaelfranca]

👍 for that!

[marcandre]

I've created PRs for check_default_type #625 and exit_on_failure? #626

I'm not sure how difficult/doable the same treatment is possible for allow_unknown_options. It's definitely harder because it influences parsing, not just checking afterwards. Moreover, I'm not confident I understand the gem enough to handle it. I made an initial draft and found a simple case I don't understand, looks like a bug to me, see #624.

I'd like to make a proposal that might make it easier to switch the default for that particular setting for modern implementations, but it's a bit too late tonight for that.