Skip to content
This repository has been archived by the owner on Apr 24, 2020. It is now read-only.

Fix test-in-docker #882

Closed
wants to merge 3 commits into from
Closed

Fix test-in-docker #882

wants to merge 3 commits into from

Conversation

dritter
Copy link
Member

@dritter dritter commented Jun 26, 2018

This fixes the test-in-docker script and adds more recent ZSH versions (5.3.1 from Debian; rest is from Ubuntu).

//cc @docwhat

Copy link
Contributor

@docwhat docwhat left a comment

Choose a reason for hiding this comment

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

Can you drop 10fa59d ('fix test-in-docker')? I fixed it for real with PR #884

The Dockerfiles look okay (I tested them on my laptop). I just had one problem with the change to the ln flags.

@@ -9,7 +9,7 @@ for rcfile in "${ZDOTDIR:-$HOME}"/.zprezto/runcoms/^README.md(.N); do
ln -nsf "$rcfile" "${ZDOTDIR:-$HOME}/.${rcfile:t}"
done

ln -s "${HOME}/p9k/powerlevel9k.zsh-theme" \
ln -sf "${HOME}/p9k/powerlevel9k.zsh-theme" \
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not use -f without -n. -n prevents ln from following symlinks.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good to know. 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

This is bigger deal with symlinks to directories. Without the -n you build loops and other fun things.

@@ -162,7 +162,7 @@ while (( $# > 0 )); do
if [[ -z "$use_framework" ]]; then
local f="$(resolve_framework "$1")"
if [[ -n "$f" ]]; then
use_framework=$f
use_framework="${f}"
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't add anything. Both are identical; when assigning a variable from a variable, quoting isn't needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. That was done by force of habit.. 🙄

if (( found <= $#frameworks )); then
echo "${frameworks[$found]}"
if (( found <= ${#frameworks} )); then
echo "${1}"
Copy link
Contributor

Choose a reason for hiding this comment

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

You're breaking the ability to find near matches.

For example the old code could handle zi and resolve it to zim. Your code will just crash trying to use docker//Dockerfile (the framework would be missing in the // part).

In addition, the old code would default to the first framework, if none was specified.

Copy link
Contributor

Choose a reason for hiding this comment

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

So it seems that sometime between ZSH 5.3 and 5.5 that this code broke. I'll look into it...

Copy link
Member Author

Choose a reason for hiding this comment

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

Hah! Good point!

Copy link
Contributor

Choose a reason for hiding this comment

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

I fixed it in #884. If you could review it, that'd be nice.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll do. And create a new PR with the new Dockerfiles on top of yours.

@@ -40,8 +40,8 @@ err()
resolve_framework() {
local f=$1 found
found=${frameworks[(In:-1:)$f*]}
if (( found <= $#frameworks )); then
echo "${frameworks[$found]}"
if (( found <= ${#frameworks} )); then
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't change anything.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, force of habit as well. 😄

@dritter
Copy link
Member Author

dritter commented Jun 27, 2018

Closed in favor of #884

@dritter dritter closed this Jun 27, 2018
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants