Closed jstucke closed 3 years ago
Merging #61 into master will decrease coverage by
0.47%
. The diff coverage is4.76%
.
@@ Coverage Diff @@
## master #61 +/- ##
==========================================
- Coverage 89.10% 88.62% -0.48%
==========================================
Files 116 116
Lines 3350 3367 +17
==========================================
- Hits 2985 2984 -1
- Misses 365 383 +18
Impacted Files | Coverage Δ | |
---|---|---|
fact_extractor/docker_extraction.py | 0.00% <0.00%> (ø) |
|
fact_extractor/unpacker/unpack.py | 92.30% <100.00%> (-0.12%) |
:arrow_down: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 1f903d8...3b22c11. Read the comment docs.
So, this introduces a CL option for the docker_extraction.py
script. What I don't yet understand from looking at the code is how these arguments can be set. The call to the script is hard coded in the Dockerfile, so does it actually make a difference when you append a command line argument to docker run
or is that argument just ignored.
Also if we assume this is only called scripted, how much sense makes the sanity check (as the error message is probably hidden away somewhere)?
Update: I read the Dockerfile reference and this seems to indeed work. Leaving only the question regarding the sanity check.
Update: I read the Dockerfile reference and this seems to indeed work. Leaving only the question regarding the sanity check.
What is more, I built the container and it seems to work as expected.
Also if we assume this is only called scripted, how much sense makes the sanity check (as the error message is probably hidden away somewhere)?
As far as I understand, the container is also intended to work as stand-alone tool and this could also prove to be useful in this regard. So not providing CLI options and error messages would make this some magic hidden feature IMHO.
As far as I understand, the container is also intended to work as stand-alone tool and this could also prove to be useful in this regard. So not providing CLI options and error messages would make this some magic hidden feature IMHO.
What is more, the change is backwards compatible and shouldn't break anything (we don't know how others use the container)
Ok. I'll add a task to add the CL argument to the README. Otherwise it's fine now.
resolves #36