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

make sub classing flay work #67

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

make sub classing flay work #67

wants to merge 1 commit into from

Conversation

tamia
Copy link

@tamia tamia commented Nov 19, 2016

I'm trying to subclass Flay, but it always calls the base class which breaks because it does not use overwritten methods. This also simplifies the code in the static methods.

@zenspider

@@ -63,7 +63,7 @@ def self.parse_options args = ARGV

OptionParser.new do |opts|
opts.banner = "flay [options] files_or_dirs"
opts.version = Flay::VERSION
opts.version = VERSION
Copy link
Member

Choose a reason for hiding this comment

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

Not sure I agree with that one... I guess it depends on what you're trying to achieve w/ the subclass?

Copy link
Author

Choose a reason for hiding this comment

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

If I build a FooFlay and it has a FooFlay::VERSION, then using fooflay -v should show fooflays version

Copy link
Member

Choose a reason for hiding this comment

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

Yeah. I get that... theoretically... but is Flay::VERSION no longer relevant? You are subclassing it so I figure you want to report it somewhere.

What does your subclass do?

Copy link
Author

Choose a reason for hiding this comment

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

the subclass overwrites the default options and some methods ... does not need the VERSION, but it is theoretically wrong to hardcode the class so I'm removing all the hardcoded classes so nobody has to come back here and to make it consistent

@@ -170,7 +170,7 @@ def self.load_plugins
# Create a new instance of Flay with +option+s.

def initialize option = nil
@option = option || Flay.default_options
@option = option || self.class.default_options
Copy link
Member

Choose a reason for hiding this comment

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

ditto? Not sure

Copy link
Author

Choose a reason for hiding this comment

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

if Fooflay overrides Flay and defines overrides default_options, then these options should be used and not the original Flay options ... atm it makes overriding do nothing

@zenspider zenspider self-assigned this Dec 1, 2016
@tamia
Copy link
Author

tamia commented Dec 12, 2016

Good to go ? ... or is there any advantage to having these hardcoded class names ?

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.

3 participants