Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
|
Hi Karl, Thanks! |
|
Now that it appears that maintainers might still exist I can look at that :) The GCLA is a bit more paperwork than the eclipse one I wasn't goign to waste time on it given the general activity level of the project earlier :) |
|
@googlebot I signed it! |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
|
@arsharma1 CLA is now signed |
| with open(fn, mode="rb") as f: | ||
| me = mfg_event_pb2.MfgEvent() | ||
| me.ParseFromString(f.read()) | ||
| from openhtf.core.test_record import TestRecord |
There was a problem hiding this comment.
Move imports to the top of the file.
Additionally, please do not import classes (or functions) out of modules; just import the module directly.
from openhtf.core import test_record
There was a problem hiding this comment.
have just dropped it, it was only needed for the type annotation
| me = mfg_event_pb2.MfgEvent() | ||
| me.ParseFromString(f.read()) | ||
| from openhtf.core.test_record import TestRecord | ||
| tr: TestRecord = mfg_event_converter.test_record_from_mfg_event(me) |
There was a problem hiding this comment.
PY2 is still technically supported for now, so please do not add type annotations.
I hope to drop PY2 support in the next few months as I really want annotations everywhere.
| @@ -463,8 +465,25 @@ class HistoryItemHandler(BaseHistoryHandler): | |||
| def get(self, file_name): | |||
| # TODO(Kenadia): Implement the history item handler. The implementation | |||
There was a problem hiding this comment.
please remove this TODO as you're fixing it!
| @@ -479,8 +498,23 @@ class HistoryAttachmentsHandler(BaseHistoryHandler): | |||
| def get(self, file_name, attachment_name): | |||
| # TODO(Kenadia): Implement the history item handler. The implementation | |||
| with open(fn, mode="rb") as f: | ||
| me = mfg_event_pb2.MfgEvent() | ||
| me.ParseFromString(f.read()) | ||
| # TODO - could use sha1 here to check? |
There was a problem hiding this comment.
TODO:
This allows our TODO search tools to identify it.
|
|
||
| fn = os.path.join(self.history_path, file_name) | ||
| if not os.path.isfile(fn): | ||
| self.write("Not found") |
There was a problem hiding this comment.
Please use single quotes for strings.
There was a problem hiding this comment.
May have missed this comment.
| self.write(desired_real[0].value_binary) | ||
| self.set_status(200) | ||
| else: | ||
| self.write("some, but no match?!") |
There was a problem hiding this comment.
self.write('Attachment not found in test.')
d12634e to
b79fc66
Compare
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
|
Updated thanks for the review |
| self.set_status(404) | ||
| return | ||
|
|
||
| with open(fn, mode="rb") as f: |
There was a problem hiding this comment.
Please use single quotes for strings.
| self.set_status(404) | ||
| return | ||
|
|
||
| with open(fn, mode="rb") as f: |
|
|
||
| fn = os.path.join(self.history_path, file_name) | ||
| if not os.path.isfile(fn): | ||
| self.write("Not found") |
There was a problem hiding this comment.
May have missed this comment.
|
Yes, sorry, fixed on memory and missed all your comments :( will do better. |
While the history handlers do need to match what has been saved on disk,
the project provides MfgEvent protobuffer writers out of the box, and
no-others, so at least let the out of box experience function as
expected, even if you would want to change this in your own
implementations.
To enable the (built in) writers, something like this is required in
your station server.
```
if __name__ == '__main__':
openhtf.util.conf.load(station_server_port='4444')
interface = mfg_inspector.MfgInspector()
interface.set_converter(mfg_event_from_test_record)
with station_server.StationServer(history_path="somepath") as server:
while 1:
test = .... #your tests here
test.add_output_callbacks(server.publish_final_state)
# explicitly match hardcoded pattern in HistoryListHandler
test.add_output_callbacks(interface.save_to_disk("somepath/mfg_event_{dut_id}_{start_time_millis}.pb"))
test.execute(test_start=user_input.prompt_for_test_start())
```
Signed-off-by: Karl Palsson <karlp@etactica.com>
b79fc66 to
5ba152a
Compare
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
|
updated thanks! |
|
Anything still blocking this? |
|
Ok, it's approved, is there anything else to do here? |
|
I have applied this to my fork and tested. It works for the most part, but the MIME types of attachments are not respected. |
|
Seems to be fixed with the following patch: diff --git a/openhtf/output/servers/station_server.py b/openhtf/output/servers/station_server.py
index 260fbc7..50c86ee 100644
--- a/openhtf/output/servers/station_server.py
+++ b/openhtf/output/servers/station_server.py
@@ -537,6 +537,7 @@ class HistoryAttachmentsHandler(BaseHistoryHandler):
# TODO: could use sha1 here to check?
desired_real = [a for a in me.attachment if a.name == attachment_name]
if len(desired_real) > 0:
+ self.set_header('Content-Type', desired_real[0].mime_type)
self.write(desired_real[0].value_binary)
self.set_status(200)
else: |
While the history handlers do need to match what has been saved on disk,
the project provides MfgEvent protobuffer writers out of the box, and
no-others, so at least let the out of box experience function as
expected, even if you would want to change this in your own
implementations.
To enable the (built in) writers, something like this is required in
your station server.
Signed-off-by: Karl Palsson karlp@etactica.com
This change is