Helpers for default variables and alias value access #1134

Merged
merged 2 commits into from Dec 3, 2013

Projects

None yet

4 participants

@koraa
koraa commented May 29, 2012

These helpers are needet for my fastfile and singlechar plugins

@robbyrussell robbyrussell merged commit 1dd9c43 into robbyrussell:master Dec 3, 2013
@mcornella
Collaborator

@koraa @mapc what are functions alias_value and try_alias_value needed for?

@koraa koraa deleted the koraa:pull_req_helpers branch Sep 4, 2014
@koraa
koraa commented Sep 4, 2014

Sorry for the delay…
To be honest, I have not looked at this code for quite some time...

Inspecting my own code, it seems that they are used for expanding the value of an alias without getting its name:

$ alias x=42
$ alias x
x=42
$ alias_value x
42

However it is a bad implementation: It does not work (the regex is wrong) and it uses sed, which means it needs to fork.
I can fix easily if you wish, but as it is not used anywhere (at leas grep tells me so) I suggest you remove it

Sorry for this strange code I produced back then oO

@mcornella
Collaborator

Haha no worries, we'll probably get rid of them in the near future (maybe in #2422 @LFDM?)

Cheers

@blueyed
Contributor
blueyed commented Dec 17, 2014

@koraa
What's the reason for the exit code of 3?

env_default could be written as:

function env_default() {
    [[ ${(t)1} == *-export* ]] && return 0
    export "$1=$2"              && return 3
}

But I would make it return 0 in the success case.

Anyway, without looking into #2422, these are things that are a bit weird to have in the omz core, especially given that they do not use Zsh features, but fork to external programs.

@robbyrussell
I suggest implementing some kind of review process, which would improve this.
(I am coming from #3359 - which was also merged without review and broke stuff)
There are quite a lot of similar issues, and a review process would not only help to prevent this, but share the work across multiple persons.

@koraa
koraa commented Dec 18, 2014

O bother, once again I am reviewing my old, shitty code ;)

I suppose this serves to check, whether the default was already set or whether it was set right now.

However, giving a return value > 0, for cases that are not really a failure is incorrect in my opinion. Besides that the return value is never used, as far as I can see.

You're indeed also correct that forking was not necessary.

Since #2422 did not touch these functions, I suggest rephrasing to:

function default() {
    test `typeset +m "$1"` || typeset -g "$1"="$2"
}

function env_default() {
    [[ ${(t)1} == *-export* ]] || export "$1=$2"
}

Sorry for that peculiar code once again.

@koraa
koraa commented Dec 18, 2014

(oh, and I'd totally love If you reviewed those six lines of code this time ;-) )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment