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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move site and collections console commands to locals #853

Merged

Conversation

mpace965
Copy link
Contributor

This is a 馃悰 bug fix.

Summary

Move the site and collections console commands to be local variables in the IRB shell.

Context

Console methods have precedence over Ruby methods with the same name. This means that any method name or local variable will be clobbered by the IRB commands site and collections. This presents an issue when using the IRB or Pry object navigation features, e.g.

3.0.6 :001 > cws site
irb: warn: can't alias source from irb_source.
#<Bridgetown::Site >
3.0.6 :002 > render
/Users/mattpace/.rvm/gems/ruby-3.0.6/gems/activesupport-7.1.2/lib/active_support/isolated_execution_state.rb:70:in `state': stack level too deep (SystemStackError)
        from /Users/mattpace/.rvm/gems/ruby-3.0.6/gems/activesupport-7.1.2/lib/active_support/isolated_execution_state.rb:38:in `[]'
        from /Users/mattpace/.rvm/gems/ruby-3.0.6/gems/activesupport-7.1.2/lib/active_support/current_attributes.rb:173:in `current_instances'
        from /Users/mattpace/.rvm/gems/ruby-3.0.6/gems/activesupport-7.1.2/lib/active_support/current_attributes.rb:100:in `instance'
        from /Users/mattpace/.rvm/gems/ruby-3.0.6/gems/activesupport-7.1.2/lib/active_support/current_attributes.rb:127:in `sites'
        from /Users/mattpace/dev/mpace965/bridgetown/bridgetown-core/lib/bridgetown-core/current.rb:11:in `site'
        from /Users/mattpace/dev/mpace965/bridgetown/bridgetown-core/lib/bridgetown-core/commands/console.rb:6:in `site'
        from /Users/mattpace/dev/mpace965/bridgetown/bridgetown-core/lib/bridgetown-core/commands/console.rb:10:in `collections'
        from /Users/mattpace/dev/mpace965/bridgetown/bridgetown-core/lib/bridgetown-core/commands/console.rb:10:in `collections'
         ... 11890 levels...
        from /Users/mattpace/.rvm/gems/ruby-3.0.6/gems/bundler-2.5.3/lib/bundler/friendly_errors.rb:117:in `with_friendly_errors'
        from /Users/mattpace/.rvm/gems/ruby-3.0.6/gems/bundler-2.5.3/exe/bundle:20:in `<top (required)>'
        from /Users/mattpace/.rvm/gems/ruby-3.0.6/bin/bundle:23:in `load'
        from /Users/mattpace/.rvm/gems/ruby-3.0.6/bin/bundle:23:in `<main>'

In this example, I've tried to execute Bridgetown::Site#render after setting the IRB workspace to site. This results in a SystemStackError because when calling collections on this line, the collections IRB command is executed instead of the collections instance method on site.

This PR moves site and collections to local variables that are passed into the main IRB workspace as a binding. This does mean that over the course of an IRB session, a user is free to initialize site or collections to something else. The upshot is that there's no longer a risk of clobbering these method names.

@jaredcwhite
Copy link
Member

TIL. Thanks for the PR @mpace965!

@jaredcwhite jaredcwhite merged commit c8ab4a1 into bridgetownrb:main Mar 15, 2024
@jaredcwhite
Copy link
Member

@mpace965 I ended up hunting around for yet a third option to facilitate this, as the previous method of setting the IRB WorkSpace binding was making the prompt in the console look really weird (it was a long Bridgetown classname instead of main). Take a look at my fix here: dc08da2

It seems like that still facilitates your example of using cws but if you think there's any issue let me know. Thanks!

@mpace965
Copy link
Contributor Author

If you were trying out the cws command in IRB, that is probably what was causing the Bridgetown class name to show up instead of main. That is actually native functionality of IRB, unrelated to this change. That part of the prompt indicates the current workspace (which the cws command changes).

That being said, it looks like defining the methods that way works just as well! Thanks for taking a look.

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.

None yet

2 participants