-
Notifications
You must be signed in to change notification settings - Fork 11
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
Refactor run.sh: added custom build-plan file and build plan checks; improved messaging #12
Conversation
Echoes build-file, build plan and version being used
Wait, I found a mistake |
run.sh
Outdated
echo "Custom build-plans file not found, using the default one" | ||
if [[ -n $1 ]]; then | ||
echo -e "\nError: Custom build plan '$1' requested, but custom '$BUILD_FILE' file not found" | ||
exit 1 |
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.
I'm thinking of a case when the above logic would prevent people from using the default build-plans file with custom build param. Currently the script would fail. Not sure if it's ok. Let's remove it. After all, if custom build plan is wrong, it will fail later at npm run build
...
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, I guess you're right.. but what would be bad about leaving the error message?
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.
But maybe that the user knows what they're doing. For example, now it's possible to pass the param directly to the npm script. E.g to build only TTFs with the rest by default:
$ docker run -it -v $(pwd):/build avivace/iosevka-build ttf::iosevka
…tom build-plans file
iosevka-build can use like this, but you patch can not. |
else | ||
# User knows what they are doing and provided custom build arguments | ||
# (manual mode) | ||
BUILD_PARAM="$1" |
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.
@avivace $1
should change to $@
to support several fonts build with in one pass.
See: be5invis/Iosevka#1340
This PR adds some messaging and checks.
From the very beginning of the
run.sh
script:npm run build
.Sample session, without custom build-plans file and build plan:
With custom build-plans file, but without a build plan:
With custom build-plans file and custom build plan: