{"action":"created","issue":{"url":"https://api.github.com/repos/therealAJ/dsl-charts/issues/2","repository_url":"https://api.github.com/repos/therealAJ/dsl-charts","labels_url":"https://api.github.com/repos/therealAJ/dsl-charts/issues/2/labels{/name}","comments_url":"https://api.github.com/repos/therealAJ/dsl-charts/issues/2/comments","events_url":"https://api.github.com/repos/therealAJ/dsl-charts/issues/2/events","html_url":"https://github.com/therealAJ/dsl-charts/issues/2","id":371374783,"node_id":"MDU6SXNzdWUzNzEzNzQ3ODM=","number":2,"title":"Code Review","user":{"login":"marquesarthur","id":1497480,"node_id":"MDQ6VXNlcjE0OTc0ODA=","avatar_url":"https://avatars1.githubusercontent.com/u/1497480?v=4","gravatar_id":"","url":"https://api.github.com/users/marquesarthur","html_url":"https://github.com/marquesarthur","followers_url":"https://api.github.com/users/marquesarthur/followers","following_url":"https://api.github.com/users/marquesarthur/following{/other_user}","gists_url":"https://api.github.com/users/marquesarthur/gists{/gist_id}","starred_url":"https://api.github.com/users/marquesarthur/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/marquesarthur/subscriptions","organizations_url":"https://api.github.com/users/marquesarthur/orgs","repos_url":"https://api.github.com/users/marquesarthur/repos","events_url":"https://api.github.com/users/marquesarthur/events{/privacy}","received_events_url":"https://api.github.com/users/marquesarthur/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":1,"created_at":"2018-10-18T06:09:45Z","updated_at":"2018-10-18T06:31:45Z","closed_at":null,"author_association":"COLLABORATOR","body":"Disclaimer: this does not affect the team grading but it's something that I usually do both in 310 or 410 as this kind of feedback may bring new ideas to the table.\r\n\r\nI took a quick look at the project and the parsing and evaluating follows the code presented in the lectures, there is a reasonable number of packages and they enclose modules that should indeed be together. the `exception` package was an overkill though. I would go with a single file exception and have all classes in it.\r\n\r\nOther than that, some parse functions are lengthy and could be broken into small and more readable functions. For instance `apply.py `:\r\n\r\n```python\r\ndef parse(self, tokenizer):\r\n        tokenizer.get_and_check_next('apply')\r\n        if tokenizer.check_next(pt.preset_table):\r\n            self.preset_name = tokenizer.get_next()\r\n        else:\r\n            try:\r\n                self.func_name = tokenizer.get_and_check_next(ft.FUNC_TABLE)\r\n            except IllegalInputException:\r\n                raise IllegalInputException(\"Call to unsupported function '{}'.\".format(tokenizer.peek()))\r\n\r\n        func = self.func_name if self.func_name else self.preset_name\r\n        print(\"Parsing {}...\".format(func))\r\n\r\n        if self.preset_name is None:\r\n            if tokenizer.maybe_match_next('using'):\r\n                self.using_variable = tokenizer.get_next()\r\n\r\n            if tokenizer.maybe_match_next('with'):\r\n                self.parameters = utils.parse_array_params(tokenizer)\r\n            else:\r\n                self.parameters = []\r\n\r\n        tokenizer.get_and_check_next('to')\r\n        self.to_variables = utils.parse_array_params(tokenizer)\r\n\r\n        if tokenizer.maybe_match_next('as'):\r\n            self.as_variables = utils.parse_array_params(tokenizer)\r\n\r\n        if len(self.as_variables) > 0 and len(self.as_variables) != len(self.to_variables):\r\n            raise IllegalInputException(\"Cardinality mismatch in AS and TO\")\r\n```\r\n\r\nCould be:\r\n\r\n```python\r\ndef parse(self, tokenizer):\r\n    self.set_preset_name ...\r\n    sef.set_func_name ...\r\n    if self.preset_name is None:\r\n        self.set_using_variable ...\r\n        self.set_parameters ...\r\n```\r\nAnd so on. By having small and more specialized methods, it's easy for new developers to understand the methods workflow and overall logic. It will add some debugging burdens but for maintainability purposes, it may be worth it. This comment applies for most of the `evaluate` functions. \r\n\r\nBe careful with some magic numbers in `functionaliy` 255 is present in many classes. What if some weird opencv update changes that to 42?\r\n\r\nPlease remember that you all did a great job, take this comments lightly. I'll be glad to discuss more about refactoring and their trade-offs if you guys are interested.\r\n\r\nLooking forward to seeing the video during the Film Fest."},"comment":{"url":"https://api.github.com/repos/therealAJ/dsl-charts/issues/comments/430891052","html_url":"https://github.com/therealAJ/dsl-charts/issues/2#issuecomment-430891052","issue_url":"https://api.github.com/repos/therealAJ/dsl-charts/issues/2","id":430891052,"node_id":"MDEyOklzc3VlQ29tbWVudDQzMDg5MTA1Mg==","user":{"login":"marquesarthur","id":1497480,"node_id":"MDQ6VXNlcjE0OTc0ODA=","avatar_url":"https://avatars1.githubusercontent.com/u/1497480?v=4","gravatar_id":"","url":"https://api.github.com/users/marquesarthur","html_url":"https://github.com/marquesarthur","followers_url":"https://api.github.com/users/marquesarthur/followers","following_url":"https://api.github.com/users/marquesarthur/following{/other_user}","gists_url":"https://api.github.com/users/marquesarthur/gists{/gist_id}","starred_url":"https://api.github.com/users/marquesarthur/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/marquesarthur/subscriptions","organizations_url":"https://api.github.com/users/marquesarthur/orgs","repos_url":"https://api.github.com/users/marquesarthur/repos","events_url":"https://api.github.com/users/marquesarthur/events{/privacy}","received_events_url":"https://api.github.com/users/marquesarthur/received_events","type":"User","site_admin":false},"created_at":"2018-10-18T06:31:45Z","updated_at":"2018-10-18T06:31:45Z","author_association":"COLLABORATOR","body":"Noooo. copy and pasta mistake. Thanks for letting me know. I'll update the original issue soon."}}
{"actor":{"display_login":"marquesarthur"}}