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

Update API to use descendant_get to return valid types #14521

Merged
merged 2 commits into from
Jul 24, 2017
Merged

Update API to use descendant_get to return valid types #14521

merged 2 commits into from
Jul 24, 2017

Conversation

jntullo
Copy link

@jntullo jntullo commented Mar 27, 2017

Mixin that returns the correct subclass from the specified string, or raises and error if it is not a subclass of that type.

Part of a refactoring effort for #14217

@miq-bot assign @Fryguy
@miq-bot add_label refactoring, wip


module ClassMethods
def class_from_string(type)
return self if type == to_s || type.nil?
Copy link
Author

Choose a reason for hiding this comment

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

@Fryguy wasn't sure we should default to returning itself if type is nil. We did this in Authentication, not sure we should generalize that though. thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

I don't like the class_from_string method as it seems to imply you can get any class, but you really can't. Perhaps, descendant_by_name or descendant_for? This seems to imply you will get the descendant class, and it mirrors the method named descendants

Copy link
Member

Choose a reason for hiding this comment

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

haha, I didn't see this until now. Honest.

@jntullo jntullo changed the title [WIP] Class from string mixin Class from string mixin Mar 31, 2017
@miq-bot miq-bot removed the wip label Mar 31, 2017
@miq-bot
Copy link
Member

miq-bot commented Apr 3, 2017

This pull request is not mergeable. Please rebase and repush.

@chessbyte
Copy link
Member

@Fryguy bump

@Fryguy
Copy link
Member

Fryguy commented Jun 14, 2017

@jntullo I commented above, but also was thiking that maybe this can be part of more_core_extensions, since it's so generic. @bdunne @jrafanie Thoughts?

task_id = type.create_in_provider_queue(manager.id, data.except('manager_resource').deep_symbolize_keys)

type = "#{manager.type}::ConfigurationScriptSource"
klass = ConfigurationScriptSource.class_from_string(type)
Copy link
Member

Choose a reason for hiding this comment

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

I had to look at the tests to know what class_from_string did.

descendent_class_from_name

Naming is hard. Technically, it should be descendent or self but that's just too many words.

I like the idea, just not sure about the name.

@jrafanie
Copy link
Member

@jntullo I commented above, but also was thiking that maybe this can be part of more_core_extensions, since it's so generic. @bdunne @jrafanie Thoughts?

Maybe? I mean, it makes sense to go there because it's a plain ruby enhancement.

@jntullo
Copy link
Author

jntullo commented Jun 16, 2017

@Fryguy @jrafanie naming really is hard. I will update the name, and yeah, I think it might make sense for more_core_extensions as well. I'll put something together

@Fryguy
Copy link
Member

Fryguy commented Jun 16, 2017

Summary of names:

descendant_by_name
descendant_for
descendant_class_from_name
descendant_from_name

I think I like the last one the best, personally. It takes @jrafanie's but removes the _class bit since it's not needed, just as the descendants method isn't called descendant_classes.

@Fryguy
Copy link
Member

Fryguy commented Jun 16, 2017

Technically, it should be descendent or self but that's just too many words.

The ancestry gem differentiates this with the word subtree, where subtree is descendants + self: https://github.com/stefankroes/ancestry#navigating-your-tree . Even so, I can't think how you could write a method name with subtree in it and still make sense. subtree_from_name isn't getting the whole tree, so it makes no sense. 😕

@bdunne
Copy link
Member

bdunne commented Jun 20, 2017

👍 to descendant_from_name

or descendant_named
or descendant_const_get

@@ -118,15 +119,6 @@ def self.build_credential_options
end
end

def self.class_from_request_data(data)
Copy link
Member

Choose a reason for hiding this comment

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

I didn't look, are there no other callers? Can you grep not only manageiq but all providers to make sure we update all callers?

I usually do this now that we have so many extracted repos..

$ bundle show rails
/Users/joerafaniello/.gem/ruby/2.4.1/gems/rails-5.0.4

Take that path and basically grep the grandparent directory so it includes not only gems but bundler git gems:

grep -ir "class_from_request_data" /Users/joerafaniello/.gem/ruby/2.4.1

Or something like that

Copy link
Author

Choose a reason for hiding this comment

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

@jrafanie yeah this is the only caller - this was a method I created for the API back in March to be replaced by this update

@jrafanie
Copy link
Member

Nice, I like it.

@jntullo jntullo changed the title Class from string mixin Update API to use descendant_get to return valid types Jun 22, 2017
@jntullo
Copy link
Author

jntullo commented Jun 30, 2017

will need new version of more_core_extensions to complete this PR

@Fryguy
Copy link
Member

Fryguy commented Jun 30, 2017

There's one other PR in the works on more_core...just want to get that one over the finish line as well before cutting a release. cc @bdunne

@bdunne
Copy link
Member

bdunne commented Jul 24, 2017

@jntullo Can you update the minimum version of more_core_extensions in the Gemfile?

@miq-bot
Copy link
Member

miq-bot commented Jul 24, 2017

Checked commits jntullo/manageiq@3ba7f66~...25d4e5b with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
6 files checked, 0 offenses detected
Everything looks fine. 🍰

@bdunne bdunne merged commit 29a1ece into ManageIQ:master Jul 24, 2017
@bdunne bdunne assigned bdunne and unassigned Fryguy Jul 24, 2017
@bdunne bdunne added the fine/no label Jul 24, 2017
@bdunne bdunne added this to the Sprint 65 Ending Jul 24, 2017 milestone Jul 24, 2017
@jntullo jntullo deleted the enhancement/type_from_string_mixin branch November 28, 2017 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants