Update readme & min-imp plugins

josehbez commented 3 years ago

Updated settings guide & Updated plugins guide
Plugin Odoo 12 tests success
Track logs /tmp/gnome-pomodoro-tracking.log
Added new methods utils

Closes #14

josehbez commented 3 years ago

Is required tokes(plugins) for run tests.

Local test OK

josehbez commented 3 years ago

Hi @gnome-pomodoro/devs Can you review this ?

kamilprusko commented 3 years ago

flake8 gives these hints:

./gp_tracking.py:250:17: E722 do not use bare 'except'
./plugins/clockify.py:34:5: E731 do not assign a lambda expression, use a def
./plugins/clockify.py:35:5: E731 do not assign a lambda expression, use a def
./plugins/clockify.py:87:13: E741 ambiguous variable name 'l'
./plugins/clockify.py:148:9: E722 do not use bare 'except'
./plugins/clockify.py:159:9: E722 do not use bare 'except'
./plugins/clockify.py:172:9: E722 do not use bare 'except'
./plugins/clockify.py:176:13: E722 do not use bare 'except'
./plugins/clockify.py:181:9: E722 do not use bare 'except'
./plugins/clockify.py:209:13: E722 do not use bare 'except'
./plugins/gpt_utils.py:51:5: E731 do not assign a lambda expression, use a def
./plugins/odoo.py:7:1: F401 '.gpt_utils.make_request' imported but unused
./plugins/odoo.py:104:5: E731 do not assign a lambda expression, use a def
./plugins/odoo.py:106:5: E731 do not assign a lambda expression, use a def
./plugins/odoo.py:198:61: E271 multiple spaces after keyword
./plugins/odoo.py:226:13: E722 do not use bare 'except'
./plugins/toggl.py:58:5: E731 do not assign a lambda expression, use a def
./plugins/toggl.py:80:13: E741 ambiguous variable name 'l'
./plugins/toggl.py:140:9: E722 do not use bare 'except'
./plugins/toggl.py:151:9: E722 do not use bare 'except'
./plugins/toggl.py:214:13: E722 do not use bare 'except'
./tests/test_clockify.py:14:34: E712 comparison to True should be 'if cond is True:' or 'if cond:'
./tests/test_gpt_plugin.py:20:20: E711 comparison to None should be 'if cond is None:'
./tests/test_gpt_plugin.py:28:20: E711 comparison to None should be 'if cond is None:'
./tests/test_odoo.py:16:34: E712 comparison to True should be 'if cond is True:' or 'if cond:'
./tests/test_toggl.py:14:34: E712 comparison to True should be 'if cond is True:' or 'if cond:'

Running tests shouldn't require env variables. Even running $ GP_TRACKING_ENV=Test pytest gives errors...

You could use mocks to fake responses from these services
I would drop pytest in favor for builtin unittest, as TestGPTPlugin is clearly a base class / mixin.

The use /tmp/gnome-pomodoro-tracking.stdout...

You could log debug info and in the test case use self.assertLogs() or in pytest use caplog.
During test redirect stdout to a buffer https://stackoverflow.com/a/39320622

println - you can specify the ouput file with with builtin print(..., file=...). But it would be better to use logging or to redirect stdout/stderr to file if there is --debug flag. See contextlib.redirect_stdout. GPTracking.cli() looks like good place to add it.

printlg - it's more common to expose logger at a module level, and setup handlers according to commandline args / config. And as I wrote before, you can easily tap into it during execution of each test (self.assertLogs() / caplog).

def make_request(method, url, **kwargs): 
        response = requests.request(method, url, **kwargs)
        if response.ok:
            return response.json()

I recommend using requests library directly. Here you assume every call is returning a json, and you lose access to response.status_code.

josehbez commented 3 years ago

flake8 gives these hints:
./gp_tracking.py:250:17: E722 do not use bare 'except'
./plugins/clockify.py:34:5: E731 do not assign a lambda expression, use a def
./plugins/clockify.py:35:5: E731 do not assign a lambda expression, use a def
./plugins/clockify.py:87:13: E741 ambiguous variable name 'l'
./plugins/clockify.py:148:9: E722 do not use bare 'except'
./plugins/clockify.py:159:9: E722 do not use bare 'except'
./plugins/clockify.py:172:9: E722 do not use bare 'except'
./plugins/clockify.py:176:13: E722 do not use bare 'except'
./plugins/clockify.py:181:9: E722 do not use bare 'except'
./plugins/clockify.py:209:13: E722 do not use bare 'except'
./plugins/gpt_utils.py:51:5: E731 do not assign a lambda expression, use a def
./plugins/odoo.py:7:1: F401 '.gpt_utils.make_request' imported but unused
./plugins/odoo.py:104:5: E731 do not assign a lambda expression, use a def
./plugins/odoo.py:106:5: E731 do not assign a lambda expression, use a def
./plugins/odoo.py:198:61: E271 multiple spaces after keyword
./plugins/odoo.py:226:13: E722 do not use bare 'except'
./plugins/toggl.py:58:5: E731 do not assign a lambda expression, use a def
./plugins/toggl.py:80:13: E741 ambiguous variable name 'l'
./plugins/toggl.py:140:9: E722 do not use bare 'except'
./plugins/toggl.py:151:9: E722 do not use bare 'except'
./plugins/toggl.py:214:13: E722 do not use bare 'except'
./tests/test_clockify.py:14:34: E712 comparison to True should be 'if cond is True:' or 'if cond:'
./tests/test_gpt_plugin.py:20:20: E711 comparison to None should be 'if cond is None:'
./tests/test_gpt_plugin.py:28:20: E711 comparison to None should be 'if cond is None:'
./tests/test_odoo.py:16:34: E712 comparison to True should be 'if cond is True:' or 'if cond:'
./tests/test_toggl.py:14:34: E712 comparison to True should be 'if cond is True:' or 'if cond:'
Running tests shouldn't require env variables. Even running $ GP_TRACKING_ENV=Test pytest gives errors...

You could use mocks to fake responses from these services

I would drop pytest in favor for builtin unittest, as TestGPTPlugin is clearly a base class / mixin.

The use /tmp/gnome-pomodoro-tracking.stdout...

You could log debug info and in the test case use self.assertLogs() or in pytest use caplog.

During test redirect stdout to a buffer https://stackoverflow.com/a/39320622

println - you can specify the ouput file with with builtin print(..., file=...). But it would be better to use logging or to redirect stdout/stderr to file if there is --debug flag. See contextlib.redirect_stdout. GPTracking.cli() looks like good place to add it.

printlg - it's more common to expose logger at a module level, and setup handlers according to commandline args / config. And as I wrote before, you can easily tap into it during execution of each test (self.assertLogs() / caplog).
def make_request(method, url, **kwargs): 
        response = requests.request(method, url, **kwargs)
        if response.ok:
            return response.json()
I recommend using requests library directly. Here you assume every call is returning a json, and you lose access to response.status_code.

@kamilprusko Thanks your feedback.

I'm working for improvement every suggestions.

josehbez commented 3 years ago

[X] flake8 based code refactoring
[X] gh-action added flake8
[X] Replaced pytest by unittest
[X] Using mock third-party API response
[X] Added flag --debug for redirect stdout, stderr and logs in a file

Hi @gnome-pomodoro/devs, Can you review this?

gnome-pomodoro / gnome-pomodoro-tracking

Update readme & min-imp plugins #15