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

Why instance_eval? #24

Closed
wants to merge 1 commit into from
Closed

Conversation

tyler-ball
Copy link

I needed to do something like this with the multi-spinner:

class MyClass

  attr_reader :config

  def doit
    ...
    spinners.register("[:spinner] :title") do |spinner|
      spinner.update(title: "#{config.value}")
    ...
  end
end

I wasn't sure why the job block was being instance_evaled. Is there a specific reason instead of just calling it?

@coveralls
Copy link

coveralls commented Aug 9, 2017

Coverage Status

Coverage decreased (-0.3%) to 94.464% when pulling 92248f1 on tyler-ball:master into 5aa42df on piotrmurach:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 94.464% when pulling 92248f1 on tyler-ball:master into 5aa42df on piotrmurach:master.

@piotrmurach
Copy link
Owner

I found it to be more flexible and intuitive as you can do the following with your example:

def doit
    ...
    spinners.register("[:spinner] :title") do
      update(title: "#{config.value}")
    ...
end

@tyler-ball
Copy link
Author

Ahh, the docs show the spinner being passed in as the first argument to the block, so thats how we structured our code. That gave us access to do spinner.update instead of just update.

LMK if you want me to continue this - I can look into the failing tests but I'm not sure why they are failing off the top of my head.

piotrmurach added a commit that referenced this pull request Aug 9, 2017
@piotrmurach
Copy link
Owner

I've added execute_job to detect the type of block you pass, so your scenario is included there. Does that work for you?

Please have a look into the multi examples. If you prefer to have more control over your multi spinner you don't have to register jobs but only the spinner format. You can later control each spinner manually. I will add some docs shortly. Unfortunately, I'm on tight schedule and have to release new version today.

@piotrmurach
Copy link
Owner

@tyler-ball I've added more docs that explain how to have full control over multi spinner registered spinners.

@piotrmurach piotrmurach closed this Aug 9, 2017
@tyler-ball
Copy link
Author

Thanks @piotrmurach !

@piotrmurach
Copy link
Owner

@tyler-ball multi spinner stuff has been released as v0.5.0

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

3 participants