Add services #26
Add services #26
Conversation
Codecov Report
@@ Coverage Diff @@
## master #26 +/- ##
==========================================
+ Coverage 69.85% 75.03% +5.18%
==========================================
Files 9 12 +3
Lines 617 677 +60
==========================================
+ Hits 431 508 +77
+ Misses 186 169 -17
Continue to review full report at Codecov.
|
aiodocker/services.py
Outdated
|
|
||
| data = {} | ||
|
|
||
| if config is not None and isinstance(config, dict): |
There was a problem hiding this comment.
this check is not required since we defined config couple lines earlier
aiodocker/services.py
Outdated
| for k, v in config.items(): | ||
| if v is not None: | ||
| data[k] = v | ||
| elif config is not None: |
There was a problem hiding this comment.
same, this elif can not be reached
aiodocker/services.py
Outdated
| data = {} | ||
|
|
||
| if config is not None and isinstance(config, dict): | ||
| for k, v in config.items(): |
There was a problem hiding this comment.
minor thing, but what do you think about {k: v for k, v in config.items() if v is not None} instead for loop ?
aiodocker/services.py
Outdated
| def __init__(self, docker): | ||
| self.docker = docker | ||
|
|
||
| async def list(self, filters: dict={}) -> list: |
There was a problem hiding this comment.
having mutable structure as default is not good idea, probably should be
filters: Optional[dict]=None and then params=filters or {}
|
@jettify thanks for the feedback. I am going to prepare a new PR. |
|
@jettify :) |
|
Asked question about typing, in #29 , what is your thoughts on this? |
|
Personally I am 👍 in adding typing. Would be useful also to verify the type of the inputs. |
|
I cleaned the typing part. |
|
All tests pass at my local setup merged with this PR. |
|
I have the same note -- please make optional arguments keyword-only. |
* Both prev/this commit passes test suite multiple times in my EC2 Ubuntu 16.04 setup with Python 3.6.1 and Docker-CE 17.03. Hm.... * NOTE: Docker emits TEXT websocket messages instead of BINARY, even for the terminal emulation.
|
Do we want to change f strings to format here? |
aiodocker/utils.py
Outdated
| @@ -57,3 +60,67 @@ def httpize(d: Optional[dict]) -> Optional[dict]: | |||
| d = decoder.decode(b'', final=True) | |||
| if d: | |||
| yield d | |||
|
|
|||
|
|
|||
| def clean_config(config: Optional[dict]) -> dict: | |||
There was a problem hiding this comment.
lets also add tests for this helpers, should be very easy, we can do this in separate PR since, current one is already large.
|
Please merge master to make sure your tests are passed on python 3.5 |
barrachri
force-pushed
the
add_services
branch
3 times, most recently
from
1812bd5
to
5da4ec9
Compare
|
You clone still conflicts with master :( |
barrachri
force-pushed
the
add_services
branch
2 times, most recently
from
ff6ea1c
to
4607847
Compare
tests/test_services.py
Outdated
|
|
||
| my_services = [] | ||
| for i in range(3): | ||
| name = f"service-{i}" |
There was a problem hiding this comment.
Tests should go on Python 3.5 too
|
Ufff, cleaned everything :) |
|
Cool! |
Updated:
Added: