{"action":"created","issue":{"url":"https://api.github.com/repos/rfrail3/tuptime/issues/16","labels_url":"https://api.github.com/repos/rfrail3/tuptime/issues/16/labels{/name}","comments_url":"https://api.github.com/repos/rfrail3/tuptime/issues/16/comments","events_url":"https://api.github.com/repos/rfrail3/tuptime/issues/16/events","html_url":"https://github.com/rfrail3/tuptime/issues/16","id":118889275,"number":16,"title":"code review?","user":{"login":"anarcat","id":796623,"avatar_url":"https://avatars.githubusercontent.com/u/796623?v=3","gravatar_id":"","url":"https://api.github.com/users/anarcat","html_url":"https://github.com/anarcat","followers_url":"https://api.github.com/users/anarcat/followers","following_url":"https://api.github.com/users/anarcat/following{/other_user}","gists_url":"https://api.github.com/users/anarcat/gists{/gist_id}","starred_url":"https://api.github.com/users/anarcat/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/anarcat/subscriptions","organizations_url":"https://api.github.com/users/anarcat/orgs","repos_url":"https://api.github.com/users/anarcat/repos","events_url":"https://api.github.com/users/anarcat/events{/privacy}","received_events_url":"https://api.github.com/users/anarcat/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":2,"created_at":"2015-11-25T17:44:09Z","updated_at":"2015-11-29T22:49:45Z","closed_at":null,"body":"would you be interested in a code review? i have a few suggestions of improvements that could be done to the code..."},"comment":{"url":"https://api.github.com/repos/rfrail3/tuptime/issues/comments/160479009","html_url":"https://github.com/rfrail3/tuptime/issues/16#issuecomment-160479009","issue_url":"https://api.github.com/repos/rfrail3/tuptime/issues/16","id":160479009,"user":{"login":"anarcat","id":796623,"avatar_url":"https://avatars.githubusercontent.com/u/796623?v=3","gravatar_id":"","url":"https://api.github.com/users/anarcat","html_url":"https://github.com/anarcat","followers_url":"https://api.github.com/users/anarcat/followers","following_url":"https://api.github.com/users/anarcat/following{/other_user}","gists_url":"https://api.github.com/users/anarcat/gists{/gist_id}","starred_url":"https://api.github.com/users/anarcat/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/anarcat/subscriptions","organizations_url":"https://api.github.com/users/anarcat/orgs","repos_url":"https://api.github.com/users/anarcat/repos","events_url":"https://api.github.com/users/anarcat/events{/privacy}","received_events_url":"https://api.github.com/users/anarcat/received_events","type":"User","site_admin":false},"created_at":"2015-11-29T22:49:45Z","updated_at":"2015-11-29T22:49:45Z","body":"alright... so here they come:\r\n\r\n* take a look at `use_scm_version` in [setuptools_scm](https://pypi.python.org/pypi/setuptools_scm) or [versionner](http://pypi.python.org/pypi/versionner) to manage the version string instead of hardcoding it\r\n* as mentionned in #14, `latest/` is a rather uncommon directory structure. i had trouble finding the actual source when i first looked at the git repo. i recommend removing the directory altogether and moving all files directly to the top-level, or at least `tuptime` itself to the toplevel, renaming `latest` to `etc`. as a last resort, rename to `src`\r\n\r\n```python\r\n\r\ntry:\r\n    import sys, os, optparse, locale, platform, subprocess, time, sqlite3\r\n    from datetime import datetime\r\nexcept Exception as e:\r\n    sys.exit('ERROR modules are not imported correctly: ' + str(e))\r\n```\r\n\r\ndo not hide exceptions from imports. this would hide what the problem actually is: it could be there is an error in one of the imported modules and the exception handling here would make it needlessly hard for the user to figure out what is wrong. those modules are all part of the standard library so there shouldn't be any such exception anyways. and if some or not, they should be on separate lines.\r\n\r\nin general, follow the style guides around: the [google style guide](https://google.github.io/styleguide/pyguide.html) is pretty amazing, but big. the canonical source is of course [pep8](https://www.python.org/dev/peps/pep-0008/) and there are great commandline tools like `pep8` and `flake` to check files for style issues. i recommend you use those, which will identify more issues than even i could find here. :)\r\n\r\n```python\r\ndef vprint(*args):\r\n    \"\"\"Print verbose information\"\"\"\r\n    if opt.verbose:\r\n        for arg in args:\r\n            sys.stdout.write(str(arg))\r\n        print('')\r\n```\r\n\r\ni wrote all this stuff very often. :) first off, you can pass all the args directly to `sys.stdout.write()` with `sys.stdout.write(*args)` to avoid that clunky loop. then i recommend you take a look at the [logging](https://docs.python.org/2/library/logging.html) module which supports writing to syslog and a bunch of similar output devices, which could be very useful for this software, instead of writing your own verbosity handler. i often map `-v` to the log level, see [bup-cron](https://github.com/anarcat/bup-cron/blob/master/bup_cron/__init__.py#L800) on how i setup the logging module there. \r\n\r\n```python\r\ndef time_conv(secs):\r\n[...]\r\n```\r\n\r\nyou went through a lot of trouble there! :) i *believe* that the datetime module can do such conversions automatically, try this out: https://github.com/anarcat/bup-cron/blob/master/bup_cron/__init__.py#L860\r\n\r\nalso, just quickly: https://github.com/rfrail3/tuptime/blob/master/latest/tuptime#L91\r\n\r\nyou should split up `main()` some more... make functions based on different ... well, functions: argument parsing, writing the database, calculating time could all be split up, and it would make code review easier.\r\n\r\nmore to come..."}}