Conversation
|
|
|
yes, sorry - this builds on #44 |
trel
force-pushed
the
36-freefunctions.m
branch
from
6caa135 to
af5bee8
Compare
|
coverage at 91% |
trel
force-pushed
the
36-freefunctions.m
branch
from
aade427 to
e34111b
Compare
- created HTTPSession class in common.py to encapsulate base URL and token - updated modules and methods to accept HTTPSession instead of separate parameters - updated test suite to use HTTPSession objects
trel
force-pushed
the
36-freefunctions.m
branch
from
e34111b to
a729cd4
Compare
|
rebased, single commit. ready for review. |
coverage, found bug in new_data_version check
coverage coverage coverage coverage
the two lines in data_objects.py are reading and writing with a ticket. not sure how to do that. |
korydraughn
left a comment
korydraughn
left a comment
There was a problem hiding this comment.
Looks good overall.
The main thing is in how errors are checked (string comparisons) and checking of HTTP status codes.
irods_http/data_objects.py
Outdated
| headers = { | ||
| "Authorization": "Bearer " + session.token, | ||
| "Content-Type": "application/x-www-form-urlencoded", | ||
| } | ||
|
|
||
| data = { | ||
| "op": "write", | ||
| "offset": offset, | ||
| "truncate": truncate, | ||
| "append": append, | ||
| "bytes": bytes_, | ||
| } | ||
|
|
||
| if parallel_write_handle != "": | ||
| data["parallel-write-handle"] = parallel_write_handle | ||
| else: | ||
| data["lpath"] = lpath | ||
|
|
||
| if resource != "": | ||
| data["resource"] = resource | ||
|
|
||
| if stream_index != -1: | ||
| data["stream-index"] = stream_index | ||
|
|
||
| r = requests.post(session.url_base + "/data-objects", headers=headers, data=data) # noqa: S113 |
There was a problem hiding this comment.
Probably want to encode this as multipart/form-data. See the tests which deal with parallel_write_init/shutdown.
trel
force-pushed
the
36-freefunctions.m
branch
from
08aa01d to
40a54e2
Compare
|
only one line untested - not sure how to generate that error text at the moment. |
trel
force-pushed
the
36-freefunctions.m
branch
from
65ec4a0 to
a2be019
Compare
alanking
left a comment
alanking
left a comment
There was a problem hiding this comment.
Given the size of this change and the fact that this hasn't been released yet, are there any areas in particular that you feel need special attention for review?
|
At this point, after the refactor to free functions, module naming, and lots of coverage work and test cleanup... I think there is only the one discussion about multipart/form-data that remains - #49 (comment) Everything else feels pretty clean, and any fixups could happen after an initial release. |
3 participants
currently a single commit beyond branch 36.m, will rebase once that is merged.
all tests pass