Workflow volume #57
Workflow volume #57
Conversation
| WORKFLOW=$(command -v "$WORKFLOW" || realpath "$WORKFLOW") | ||
| if ! test -f "$WORKFLOW"; then | ||
| logger -p user.error -t $TASK "invalid workflow '$WORKFLOW'" | ||
| exit 3 | ||
| fi | ||
| logger -p user.notice -t $TASK "using workflow '$WORKFLOW':" | ||
| ocrd_format_workflow | logger -p user.notice -t $TASK | ||
| if test "${WORKFLOW#/workflows/}" = "$WORKFLOW"; then |
There was a problem hiding this comment.
I do not understand the check here cause of Line 48 and i think that needs a comment.
So with this parameter substitution it looks like the file test Line 48 will fail or this check will not be successful.
IMO for example:
With /workflows/ocr-workflow-default.sh as $WORKFLOW Line 48 will successful.
Line 54 "ocr-workflow-default.sh" = "/workflows/ocr-workflow-default.sh" will fail.
There was a problem hiding this comment.
That's precisely the point. The check trivially looks whether the realpath of the workflow starts with /workflows and thus, needs no further action. If it does not, then it gets copied into the workdir under a default name, to ensure it will be accessible.
There was a problem hiding this comment.
Alone that already the inquiry came from me would be worth to write comment there cause it is not easy to read (for none linux power users those for whom parameter substitution syntax has not yet become flesh and blood 😉 ) and understand why you do that here.
There was a problem hiding this comment.
The problem is that by this logic, I would have to second every other statement in the script with an explanatory comment. Code is also documentation in itself. Only complex / non-trivial code bits should be commented.
If you encounter an unknown construct in a (new) language, it's up to you to visit the documentation first. This expansion test is very common in shell scripts.
Feel free to make a suggestion with a better formulation (preferbly inside the condition, i.e. from the perspective after the test succeeded).
There was a problem hiding this comment.
Feel free to make a suggestion with a better formulation (preferbly inside the condition, i.e. from the perspective after the test succeeded).
Maybe a little bit simpler otherwise i would separate the variable or leave a small comment.
| if test "${WORKFLOW#/workflows/}" = "$WORKFLOW"; then | |
| if test "${WORKFLOW:0:11}" != "/workflow/"; then |
IMO not only knowledge was required, you also had to think outside the box. I think code should be clearly visible without thinking outside the box. But I understand your points and I can also live with the current state.
There was a problem hiding this comment.
I don't know what you mean by thinking outside the box here. This substitution test is straightforward and very common (e.g. you can see tons of these tests in every autoconf script). Your offeset-length test would also work (with plural s), but is less idiomatic.
I just added a comment – happy now?
(You still need to unrequest changes, otherwise the PR is still blocked.)
There was a problem hiding this comment.
Thx sorry for the circumstances 🤐
| WORKFLOW=$(command -v "$WORKFLOW" || realpath "$WORKFLOW") | ||
| if ! test -f "$WORKFLOW"; then | ||
| logger -p user.error -t $TASK "invalid workflow '$WORKFLOW'" | ||
| exit 3 | ||
| fi | ||
| logger -p user.notice -t $TASK "using workflow '$WORKFLOW':" | ||
| ocrd_format_workflow | logger -p user.notice -t $TASK | ||
| if test "${WORKFLOW#/workflows/}" = "$WORKFLOW"; then |
There was a problem hiding this comment.
Feel free to make a suggestion with a better formulation (preferbly inside the condition, i.e. from the perspective after the test succeeded).
Maybe a little bit simpler otherwise i would separate the variable or leave a small comment.
| if test "${WORKFLOW#/workflows/}" = "$WORKFLOW"; then | |
| if test "${WORKFLOW:0:11}" != "/workflow/"; then |
IMO not only knowledge was required, you also had to think outside the box. I think code should be clearly visible without thinking outside the box. But I understand your points and I can also live with the current state.
fix #46
also,