Skip to content

ensure-lib-src-rs-exist: Updated the script to check if the stub file is empty #616

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
coriolinus merged 3 commits into from
Aug 30, 2018
Merged

ensure-lib-src-rs-exist: Updated the script to check if the stub file is empty #616

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fb3f04b
ensure-lib-src-rs-exist: Updated the script to check if the stub file…
ZapAnton Aug 9, 2018
fffeb5c
ensure-lib-src-rs-exist: The script is now evoked via bash in the Travis
ZapAnton Aug 20, 2018
3a1aea4
ensure-lib-src-rs-exist: Updated script for consistent indenting
ZapAnton Aug 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
language: rust
script:
- "./_test/check-exercises.sh"
- "sh ./_test/ensure-lib-src-rs-exist.sh"
- "bash ./_test/ensure-lib-src-rs-exist.sh"
- "sh ./_test/ensure-stubs-compile.sh"
- "sh ./_test/count-ignores.sh"
- "./bin/fetch-configlet"
Expand Down
46 changes: 38 additions & 8 deletions _test/ensure-lib-src-rs-exist.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,46 @@ repo=$(cd "$(dirname "$0")/.." && pwd)

missing=""

empty_stub=""

check_status=0

IGNORED_EXERCISES=(
"two-fer" #deprecated
"nucleotide-codons" #deprecated
"hexadecimal" #deprecated
)

for dir in $repo/exercises/*/; do
exercise=$(basename "$dir")
Copy link
Copy Markdown
Member

@petertseng petertseng Aug 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are these changes purely for re-indenting the lines?

I would ask that re-indent be its own commit (or not included at all)

Copy link
Copy Markdown
Contributor Author

@ZapAnton ZapAnton Aug 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@petertseng petertseng Aug 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Member

@petertseng petertseng Aug 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if [ ! -f $dir/src/lib.rs ]; then
# https://github.com/exercism/rust/pull/270
echo "$exercise is missing a src/lib.rs; please create one (an empty file is acceptable)"
missing="$missing\n$exercise"
fi
exercise=$(basename "$dir")

if [ ! -f "$dir/src/lib.rs" ]; then
# https://github.com/exercism/rust/pull/270
echo "$exercise is missing a src/lib.rs; please create one (an empty file is acceptable)"
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
Copy link
Copy Markdown
Member

@petertseng petertseng Aug 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(despite the above comment, I understand that these are new lines)

echo "$exercise has src/lib.rs stub file, but it is empty."
empty_stub="$empty_stub\n$exercise"
fi
fi
done

if [ -n "$missing" ]; then
echo "Exercises missing src/lib.rs:$missing"
exit 1
printf "\nExercises missing src/lib.rs:$missing\n"

check_status=1
fi

if [ -n "$empty_stub" ]; then
printf "\nExercises with empty src/lib.rs stub file:$empty_stub\n"

check_status=1
fi

if [ "$check_status" -ne 0 ]; then
Copy link
Copy Markdown
Member

@petertseng petertseng Aug 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could potentially be written as exit $check_status - was this intentionally written the proposed way for clarity, or some other reason?

Copy link
Copy Markdown
Contributor Author

@ZapAnton ZapAnton Aug 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two reasons actually:

  • For clarity
  • Separation of concern: check_status is used for internal script logic exit 0/1 is used for external system(Travis in this case)

If this feels like unnecessary logic to you I will change it to exit $check_status

exit 1
else
exit 0
fi