GitHub PullRequestReviewCommentEvent at 2014-04-25 14:13:55
Repository: devassistant
Actor: tradej
Payload: {"comment":{"url":"https://api.github.com/repos/bkabrda/devassistant/pulls/comments/11997837","id":11997837,"diff_hunk":"@@ -280,6 +280,65 @@ def resolve(cls, *args):\n \n \n @register_manager\n+class HomebrewPackageManager(PackageManager):\n+    \"\"\"Package manager for managing OSX packages by homebrew.\"\"\"\n+    permission_prompt = \"Installing {num} package{plural} by Homebrew. Is this ok?\"\n+    shortcut = 'homebrew'\n+\n+    c_homebrew = 'brew'\n+\n+    @classmethod\n+    def install(cls, *args):\n+        cmd = [cls.c_homebrew, 'install']\n+        quoted_pkgs = map(lambda pkg: '\"{pkg}\"'.format(pkg=pkg), args)\n+        cmd.extend(quoted_pkgs)\n+        try:\n+            ClHelper.run_command(' '.join(cmd), ignore_sigint=True)\n+            return args\n+        except exceptions.ClException:\n+            return False\n+\n+    @classmethod\n+    def is_pkg_installed(cls, dep):\n+        logger.info('Checking for presence of {0}...'.format(dep),\n+                    extra={'event_type': 'dep_check'})\n+        if not getattr(cls, '_installed', None):\n+            query = ClHelper.run_command(' '.join([cls.c_homebrew, 'list']))\n+            cls._installed = query.split('\\n')\n+        search = filter(lambda e: e.startswith(dep), cls._installed)\n+        if search:\n+            logger.info('Found {0}'.format(search[0]), extra={'event_type': 'dep_found'})\n+        else:\n+            logger.info('Not found, will install', extra={'event_type': 'dep_not_found'})\n+\n+        return len(search) > 0\n+\n+\n+    @classmethod\n+    def works(cls):\n+        try:\n+            ClHelper.run_command('which brew')\n+            return True\n+        except exceptions.ClException:\n+            return False\n+\n+    @classmethod\n+    def resolve(cls, *args):\n+      logger.info('Resolving Homebrew dependencies ...')\n+      for pkg in args:\n+        logger.debug('Looking at {0}'.format(pkg))\n+\n+      logger.debug('Installing/Updating:')\n+      to_install = []","path":"devassistant/package_managers.py","position":53,"original_position":53,"commit_id":"e6d1b5b12909dec7a79726b4b676f78706def009","original_commit_id":"e6d1b5b12909dec7a79726b4b676f78706def009","user":{"login":"tradej","id":1138123,"avatar_url":"https://avatars.githubusercontent.com/u/1138123?","gravatar_id":"e441cf0aa734b0b4a0bb777886d51bd5","url":"https://api.github.com/users/tradej","html_url":"https://github.com/tradej","followers_url":"https://api.github.com/users/tradej/followers","following_url":"https://api.github.com/users/tradej/following{/other_user}","gists_url":"https://api.github.com/users/tradej/gists{/gist_id}","starred_url":"https://api.github.com/users/tradej/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/tradej/subscriptions","organizations_url":"https://api.github.com/users/tradej/orgs","repos_url":"https://api.github.com/users/tradej/repos","events_url":"https://api.github.com/users/tradej/events{/privacy}","received_events_url":"https://api.github.com/users/tradej/received_events","type":"User","site_admin":false},"body":"The double list comprehension on line ```337``` is brutal :) . I see what you are doing here, but I would recommend going like this:\r\n```python\r\nto_install = set()\r\nfor pkg in args:\r\n    query = ClHelper.run_command(' '.join([cls.c_homebrew, 'deps -n', pkg]))\r\n    to_install.update(query.split('\\n'))\r\n\r\nreturn list(to_install)\r\n```\r\nThis approach assumes that the ClHelper invocation returns list of packages delimited by newlines. Using ```set()``` ensures that no package is listed twice.","created_at":"2014-04-25T14:13:55Z","updated_at":"2014-04-25T14:13:55Z","html_url":"https://github.com/bkabrda/devassistant/pull/204#discussion_r11997837","pull_request_url":"https://api.github.com/repos/bkabrda/devassistant/pulls/204","_links":{"self":{"href":"https://api.github.com/repos/bkabrda/devassistant/pulls/comments/11997837"},"html":{"href":"https://github.com/bkabrda/devassistant/pull/204#discussion_r11997837"},"pull_request":{"href":"https://api.github.com/repos/bkabrda/devassistant/pulls/204"}}}}