ensure-lib-src-rs-exist: Updated the script to check if the stub file is empty#616
Conversation
|
Seems like the script could not be run, because of the syntax error in the list creation. |
|
Arrays are bash syntax not available in |
|
For consistency perhaps every other script should be run with the |
|
I don't have strong feelings on that either way. Just be careful not to
break something which uses `sh`-specific features not available in bash.
…On Sat, Aug 18, 2018 at 4:42 AM Zapolsky Anton ***@***.***> wrote:
For consistency perhaps every other script should be run with the bash
too?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#616 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AHdeTtHjHOM1BI6mJLkM9lKkF_AzwUitks5uR9NjgaJpZM4V2YqU>
.
|
|
The script now works as intended, and Travis fails because there are still exercises which lack the template in the stub file. For now I will wait for a comment from maintainers whether this should be rebased or simply merged when other templates are added (or any more changes should be made). Making the syntax and calls of the Travis shell scripts consistent would be moved to the other issue, since they are not a concern in this specific PR. |
petertseng
left a comment
petertseng
left a comment
There was a problem hiding this comment.
Making the syntax and calls of the Travis shell scripts consistent would be moved to the other issue, since they are not a concern in this specific PR.
Yup.
If this were a computer where performance were important, I would only use bash where bash-specific features are necessary, and use sh for all other instances. Since this is not one such context, I suppose I don't care. So whoever chooses to do the work gets to choose.
| ) | ||
|
|
||
| for dir in $repo/exercises/*/; do | ||
| exercise=$(basename "$dir") |
There was a problem hiding this comment.
are these changes purely for re-indenting the lines?
I would ask that re-indent be its own commit (or not included at all)
There was a problem hiding this comment.
May I ask why?
The lines I have added for this PR have their own indent and if i leave the above line as they were, it would hurt readability.
There was a problem hiding this comment.
May I ask why?
Well, I thought that if a line is changed in a way that isn't consistent with the commit message, it would cause confusion, make it harder to understand the git history, and make it harder to review. Now a reviewer needs to think to see whether a line was changed only for reindenting, or if it has a change other than reindent. A separate commit makes it clear to the reviewer.
if i leave the above line as they were, it would hurt readability.
indeed, so I think I should not suggest they are left as they are. Putting the indentation change in its own commit should accomplish both the goals though (understand history, and keep the indentation consistent)
There was a problem hiding this comment.
Note that I understand I am causing non-trivial hardship here, by asking for the re-indenting be its own commit. It is not necessarily the most pleasant experience to have to redo the commits to split them in two.
I offer this remedy:
Since I'm the one who cares most about this, I offer to make it happen. However I can only execute this offer at a time more convenient to me. Luckily we still have time before all exercises have their stubs, so we are not in a huge hurry.
There is no obligation to accept this offer, of course.
| missing="$missing\n$exercise" | ||
| else | ||
| #Check if the stub file is empty | ||
| if [ ! -s "$dir/src/lib.rs" ] || [ "$(cat "$dir/src/lib.rs")" == "" ] && [[ " ${IGNORED_EXERCISES[*]} " != *"$exercise"* ]]; then |
There was a problem hiding this comment.
(despite the above comment, I understand that these are new lines)
| check_status=1 | ||
| fi | ||
|
|
||
| if [ "$check_status" -ne 0 ]; then |
There was a problem hiding this comment.
this could potentially be written as exit $check_status - was this intentionally written the proposed way for clarity, or some other reason?
There was a problem hiding this comment.
Two reasons actually:
- For clarity
- Separation of concern:
check_statusis used for internal script logicexit 0/1is used for external system(Travis in this case)
If this feels like unnecessary logic to you I will change it to exit $check_status
ZapAnton
force-pushed
the
ensure_stub_exists
branch
3 times, most recently
from
32ce5cd to
a5a33bc
Compare
|
@petertseng I have rewritten my commits to extract the indenting changes into separate commit. Does it look good to you? |
coriolinus
left a comment
coriolinus
left a comment
There was a problem hiding this comment.
I have a mild preference in favor of exit $check_status, but it's not strong enough to preclude approval.
I will not merge this until it passes Travis. It will be necessary to merge master or rebase as we add exercise stubs in order to trigger reruns with appropriate context.
ZapAnton
force-pushed
the
ensure_stub_exists
branch
from
a5a33bc to
3a1aea4
Compare
|
I have rebased against the |
3 participants
As discussed in #551 the script in this PR now has a check to ensure that the stub files of the exercises
are not empty.
The reasons why
ensure-lib-src-rs-exist.shwas chosen for this PR and notensure-stubs-compile.share:ensure-stubs-compile.sh- it should check if the stub compiles, even if it is emptyensure-lib-src-rs-exist.shchecks for the stub and thinks it exists only because the file exists (even though it could be empty) is sort of false-positiveThe
[ "$(cat "$dir/src/lib.rs")" == "" ]line was added for the check, because whenrustfmtworks on an empty file it inserts the0asymbol in it, and the-sflag check is useless in this case.In case this PR is merged, closes #551