Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow for nested no_commands blocks. #697

Merged

Conversation

tmfnll
Copy link
Contributor

@tmfnll tmfnll commented Dec 11, 2019

Allow for nested no_commands blocks. 🌈

Background

In certain circumstances we may wish to nest no_commands blocks.

However, since when exiting the no_commands block the @no_commands
instance var is reset to false, we get unexpected behaviour.

For example, this will show warnings stating that the method foo is missing usage and a description:

class Foo < Thor

  no_method do
    attr_reader :bar

    def foo
      'foo!'
    end
  end
end

This particular example is trivially solved but things get more difficult when, say, including modules.

# lib/thor/base.rb
def no_commands
  @no_commands = true
  yield
ensure
   @no_commands = false
end

Solution

This change uses a NestedContext object to track the depth of the no_command blocks and ensure that we only start creating commands again once we've left all of them.

Comments

  • I'm definitely not sold on the name NestedContext

@tmfnll tmfnll force-pushed the tack-no_commands-blocks-using-stack branch from 01e694c to 2c5c6e4 Compare December 11, 2019 17:25
In certain circumstances we may wish to nest `no_commands` blocks.
For example, if we want to import any module which includes both methods
and `attr_reader` invocations.

However, since when exiting the no_commands block the @no_commands
instance var is reset to `false` we get unexpected behaviour.

This change uses a `NestedContext` object to track the depth of the
no_command blocks and ensure that we only start creating commands again
once we've left all of them.
@tmfnll tmfnll force-pushed the tack-no_commands-blocks-using-stack branch from 2c5c6e4 to 8e296f6 Compare December 11, 2019 19:45
@rafaelfranca
Copy link
Member

Thank you for the pull request. I think this solution is too complex just to allow nested no_commands. Can you explain why you need nested no_commands?

@tmfnll
Copy link
Contributor Author

tmfnll commented Dec 11, 2019

Hello, I have been working on refactoring some scripts for a Rails application and wanted to make use of the ActiveModel validation API. So I included the ActiveModel::Validations module which created the nested no_commands via an attr_reader.

@rafaelfranca rafaelfranca merged commit d9972d7 into rails:master Dec 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants