{"action":"created","issue":{"url":"https://api.github.com/repos/ManageIQ/manageiq/issues/8886","repository_url":"https://api.github.com/repos/ManageIQ/manageiq","labels_url":"https://api.github.com/repos/ManageIQ/manageiq/issues/8886/labels{/name}","comments_url":"https://api.github.com/repos/ManageIQ/manageiq/issues/8886/comments","events_url":"https://api.github.com/repos/ManageIQ/manageiq/issues/8886/events","html_url":"https://github.com/ManageIQ/manageiq/pull/8886","id":156252250,"number":8886,"title":"SupportsFeatureMixin","user":{"login":"durandom","id":161888,"avatar_url":"https://avatars.githubusercontent.com/u/161888?v=3","gravatar_id":"","url":"https://api.github.com/users/durandom","html_url":"https://github.com/durandom","followers_url":"https://api.github.com/users/durandom/followers","following_url":"https://api.github.com/users/durandom/following{/other_user}","gists_url":"https://api.github.com/users/durandom/gists{/gist_id}","starred_url":"https://api.github.com/users/durandom/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/durandom/subscriptions","organizations_url":"https://api.github.com/users/durandom/orgs","repos_url":"https://api.github.com/users/durandom/repos","events_url":"https://api.github.com/users/durandom/events{/privacy}","received_events_url":"https://api.github.com/users/durandom/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/ManageIQ/manageiq/labels/providers","name":"providers","color":"5319e7"},{"url":"https://api.github.com/repos/ManageIQ/manageiq/labels/refactoring","name":"refactoring","color":"fbca04"}],"state":"open","locked":false,"assignee":{"login":"blomquisg","id":14183,"avatar_url":"https://avatars.githubusercontent.com/u/14183?v=3","gravatar_id":"","url":"https://api.github.com/users/blomquisg","html_url":"https://github.com/blomquisg","followers_url":"https://api.github.com/users/blomquisg/followers","following_url":"https://api.github.com/users/blomquisg/following{/other_user}","gists_url":"https://api.github.com/users/blomquisg/gists{/gist_id}","starred_url":"https://api.github.com/users/blomquisg/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/blomquisg/subscriptions","organizations_url":"https://api.github.com/users/blomquisg/orgs","repos_url":"https://api.github.com/users/blomquisg/repos","events_url":"https://api.github.com/users/blomquisg/events{/privacy}","received_events_url":"https://api.github.com/users/blomquisg/received_events","type":"User","site_admin":false},"milestone":null,"comments":9,"created_at":"2016-05-23T10:48:20Z","updated_at":"2016-05-23T14:33:24Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/ManageIQ/manageiq/pulls/8886","html_url":"https://github.com/ManageIQ/manageiq/pull/8886","diff_url":"https://github.com/ManageIQ/manageiq/pull/8886.diff","patch_url":"https://github.com/ManageIQ/manageiq/pull/8886.patch"},"body":"This is a first stab at refactoring `is_available?` and `validate_` calls.\r\n\r\nBefore this a `validate_<feature>` call had to be implemented as an instance method and had to return a hash or nil. `{ available: true, message: 'Not available' }`\r\nIn case nil is returned, the feature is not available. \r\nIn case a Hash is returned and available is false, then the feature is basically available, but for the reason in :message its currently not. \r\nIf :available is true, the feature is available right now.\r\nMy concern with this is \r\n* wording: validate_ sounds like rails validation, but we are not validating the object itself\r\n* return value: either a nil value could be returned or a hash\r\n* an object/record needs to be present to check the general validity of a feature\r\n\r\nThe `validate_` method would generally be used by the UI to test the availability of an operation/feature. \r\nThis would be done by a pair of methods: `is_available?(operation)` and `is_available_now_error_message(operation)`\r\nMy concern with this is:\r\n* wording: `is_available?` does not communicate what is available and the side effect that it depends on internal state of the object\r\n* input is keyword which cannot be introspected like a method without any arguments (what can I query for?)\r\n* an object/record needs to be present to check the general validity of a feature\r\n\r\nA first refactor was the extraction into a [AvailabilityMixin](https://github.com/ManageIQ/manageiq/pull/7062) and a rename of those `available` method names: https://github.com/ManageIQ/manageiq/pull/3596\r\n\r\nI propose a more DSL like approach that adds a `supports` class method, which defines 3 methods\r\n* A class level method that returns the general support of a feature\r\n* An instance level method that answers if the feature is supported by this instance\r\n* Another instance level method that returns the reason for being not supported by the instance\r\n\r\n```ruby\r\nclass Animal\r\n  supports :die\r\n  supports :fly, false\r\nend\r\n\r\nclass Bird < Animal\r\n  supports :fly do |bird|\r\n    if bird.penguin? \r\n      \"Penguins can't fly\"\r\n    else\r\n      true\r\n    end\r\n  end\r\nend\r\n\r\nAnimal.supports_die? # true\r\nAnimal.new.supports_die? # true\r\nAnimal.supports_fly? # false\r\nBird.supports_fly? # true\r\nBird.new(penguin: true).support_fly? # false\r\nBird.new(penguin: true).unsupported_fly_reason # 'Penguins can't fly\r\n```\r\n\r\nAs an example I refactored `live_migrate`\r\n\r\n@Fryguy @martinpovolny @mfalesni what do you think of this?\r\n\r\n@miq-bot add_label refactoring"},"comment":{"url":"https://api.github.com/repos/ManageIQ/manageiq/issues/comments/220997577","html_url":"https://github.com/ManageIQ/manageiq/pull/8886#issuecomment-220997577","issue_url":"https://api.github.com/repos/ManageIQ/manageiq/issues/8886","id":220997577,"user":{"login":"chessbyte","id":82161,"avatar_url":"https://avatars.githubusercontent.com/u/82161?v=3","gravatar_id":"","url":"https://api.github.com/users/chessbyte","html_url":"https://github.com/chessbyte","followers_url":"https://api.github.com/users/chessbyte/followers","following_url":"https://api.github.com/users/chessbyte/following{/other_user}","gists_url":"https://api.github.com/users/chessbyte/gists{/gist_id}","starred_url":"https://api.github.com/users/chessbyte/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/chessbyte/subscriptions","organizations_url":"https://api.github.com/users/chessbyte/orgs","repos_url":"https://api.github.com/users/chessbyte/repos","events_url":"https://api.github.com/users/chessbyte/events{/privacy}","received_events_url":"https://api.github.com/users/chessbyte/received_events","type":"User","site_admin":false},"created_at":"2016-05-23T14:33:24Z","updated_at":"2016-05-23T14:33:24Z","body":"@Fryguy with your approach, you lose the ability to override something supported on a base class and not implemented on a subclass."}}
{"actor":{"display_login":"chessbyte"}}