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

Refactor to move more builder methods out into Command classes #34

Merged
merged 3 commits into from
Aug 6, 2013

Conversation

samnang
Copy link
Contributor

@samnang samnang commented Jul 27, 2013

See #11

I have moved all of builder's methods into commands except black_hole because it's so simple 1 line, so that's why I'm not sure it worths to move it or not?

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 25990b3 on samnang:move_builder_methods_into_commands into 0869d4a on avdi:master.


def call
defer do |subject|
methods = @class_to_mimic.instance_methods(@include_super) -
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it better to have attr_reader for @class_to_mimic and @include_super?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say yes. The closer the #call method contents is to the extracted method's contents the happier I am.

@avdi
Copy link
Owner

avdi commented Jul 28, 2013

Thanks for this, this is really helpful. Do you mind making the inline-noted changes?

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling a49f5b7 on samnang:move_builder_methods_into_commands into 0869d4a on avdi:master.

module Naught
class NullClassBuilder
module Commands
class Traceable < Naught::NullClassBuilder::Command
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you think by reducing some nested scopes to be this instead?

module Naught::NullClassBuilder::Commands
  class Traceable < Naught::NullClassBuilder::Command
    # ...
  end 
end

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine with me.

On Sun, Jul 28, 2013 at 12:17 PM, Samnang Chhun notifications@github.comwrote:

In lib/naught/null_class_builder/commands/traceable.rb:

@@ -0,0 +1,24 @@
+require 'naught/null_class_builder/command'
+
+module Naught

  • class NullClassBuilder
  • module Commands
  •  class Traceable < Naught::NullClassBuilder::Command
    

what do you think by reducing some nested scopes to be this instead:

module Naught::NullClassBuilder::Commands
class Traceable < Naught::NullClassBuilder::Command
# ...
end end


Reply to this email directly or view it on GitHubhttps://github.com//pull/34/files#r5440295
.

Avdi Grimm
http://avdi.org

I only check email twice a day. to reach me sooner, go to
http://awayfind.com/avdi

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 2dc6e30 on samnang:move_builder_methods_into_commands into 0869d4a on avdi:master.

@samnang
Copy link
Contributor Author

samnang commented Jul 30, 2013

Done for changes. Do you have other things to me to look at before merging this?

@avdi
Copy link
Owner

avdi commented Aug 6, 2013

You are awesome.

avdi pushed a commit that referenced this pull request Aug 6, 2013
Refactor to move more builder methods out into Command classes
@avdi avdi merged commit 5641a7c into avdi:master Aug 6, 2013
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