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

test-in-docker: fix with newer ZSH versions #884

Closed
wants to merge 11 commits into from
Closed

test-in-docker: fix with newer ZSH versions #884

wants to merge 11 commits into from

Conversation

docwhat
Copy link
Contributor

@docwhat docwhat commented Jun 27, 2018

The way I was filtering out entries in the frameworks array stopped working in newer versions of ZSH; it would convert the array into a string (you could see it with typeset -p frameworks)

So I rewrote it.

I don't see anything in the release notes for ZSH that would explain this and I didn't find any option that would restore this behavior.

I also added a bunch of debugging and warning stuff to catch problems in the future.

And there is a --dry-run flag now.

Related: #882 (cc: @dritter)

The way I was filtering out entries in the frameworks array stopped
working in newer versions of ZSH; it would convert the array into a
string (you could see it with `typeset -p frameworks`)

So I rewrote it.

I don't see anything in the release notes for ZSH that would explain
this and I didn't find any option that would restore this behavior.

Related: #882
Marking variables as readonly is helpful for debugging and preventing
problems.
@docwhat
Copy link
Contributor Author

docwhat commented Jun 27, 2018

@dritter I wouldn't mind a review on this, if you have time.

@docwhat docwhat mentioned this pull request Jun 27, 2018
test-in-docker Outdated
frameworks=${(@)frameworks:#base-*}
for i in {$#frameworks..1}; do
# Remove all base entries
[[ "${frameworks[$i]}" = base-* ]] && frameworks[$i]=()
Copy link
Member

Choose a reason for hiding this comment

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

Just a stylistic note: I would go here with double equals. That is more readable IMHO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 3c27414

test-in-docker Outdated
@@ -6,6 +6,7 @@ set -eu
default_version='4.3.11'
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should set the minimum version to something like 5.3 in regard of #877

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I started to do that in a94df3d but then remembered why I have it the way I do.

The default should be the oldest version of zsh we support, not newest.

Either way, it is easier to change now and it has the same lookup properties the command line does.

Copy link
Member

Choose a reason for hiding this comment

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

Well. The oldest supported version of ZSH is 5.3 in next (at least in the battery segment). Otherwise it is 5.1.

@dritter
Copy link
Member

dritter commented Jun 27, 2018

Btw. Did you see my questions in the chat?

@@ -1,4 +1,4 @@
FROM ubuntu:17.04
FROM ubuntu:17.10
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. 17.04 (and 17.10) are not LTS versions; they will go away.

@dritter dritter added this to To do in v0.6.6 Jul 23, 2018
@dritter dritter moved this from To do to In progress in v0.6.6 Jul 23, 2018
@dritter dritter moved this from In progress to Done in v0.6.6 Jul 23, 2018
@dritter dritter moved this from Done to In progress in v0.6.6 Jul 23, 2018
@dritter dritter mentioned this pull request Aug 5, 2018
@dritter dritter moved this from In progress to Done in v0.6.6 Aug 9, 2018
@bhilburn
Copy link
Member

Merged into master as part of #944, and will be part of the v0.6.6 release!

@bhilburn bhilburn closed this Aug 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
v0.6.6
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants