Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
PR for #18: Improve error handling #19
base: main
Are you sure you want to change the base?
PR for #18: Improve error handling #19
Changes from 9 commits
796332a
fc0e95e
182cce5
efc8d95
404e2d7
af61c3e
98dc2cb
6240a8d
888557d
30a11ed
f61cf15
cd9fdbf
f263307
584c3d4
9e7d338
1f6cb71
5e466e3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One general comment: we sometime don't need very much comments, the scripts should be self documenting. Maybe we can tidy up the scripts for a little bit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this makes sense. I can remove some comments (and please let me know if you see specific ones that you think should be removed). Thanks @ShiqiYang2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My sense is
ls
andgrep
are slow. If we have many many small files as output (maybe for example, television transcripts), then this need to take a lot of time to process. Shall we consider using more efficient methods to handle file lists, such as directly using regular expressions within thefind
command.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar comment to the all other scripts below. Also, I don't know whether we need to separate this functionality into a seperate part of script? Say, we can do separate this part into
check_file_dependencies.sh
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ShiqiYang2022 , I will investigate different approaches for handling the file lists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ShiqiYang2022 I just pushed a change in 1f6cb71 to replace the code that handles the file lists. Let me know what you think. In the same commit I also added a minor fix to the Program Error warnings so that they skip a line when printing in the terminal. Thanks!