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

Handle environment variables with = signs correctly #2403

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@crmne

crmne commented Sep 18, 2015

Scenario:

I need to run this line:

export THEANO_FLAGS='floatX=float32,device=gpu,nvcc.fastmath=True'

but the current implementation of the export function doesn't export anything, since the tr command would split the string at all the occurrences of the equal sign and end up having no case in the switch statement.

Solution

By using sed we ensure that the string gets split at only the first occurrence of the equal sign, emulating more carefully bash's behavior.

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Sep 18, 2015

Member

This sounds okay, but I'd like a comment codifying this, so someone touching it (and I'll probably touch every single sed invocation in our codebase shortly) knows this is intentional.

Member

faho commented Sep 18, 2015

This sounds okay, but I'd like a comment codifying this, so someone touching it (and I'll probably touch every single sed invocation in our codebase shortly) knows this is intentional.

@faho faho closed this in 20e96df Sep 24, 2015

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Sep 24, 2015

Member

Hi, I've now pushed a slightly different solution using our new cool string tool, including a comment.

Still thanks for your contribution!

Member

faho commented Sep 24, 2015

Hi, I've now pushed a slightly different solution using our new cool string tool, including a comment.

Still thanks for your contribution!

@faho faho added this to the next-2.x milestone Sep 24, 2015

@lledey

This comment has been minimized.

Show comment
Hide comment
@lledey

lledey Oct 12, 2015

Contributor

There is an issue with this fix, the \n is interpreted as "\n" instead of a line return.
I can't make a pull request but the fix is very simple.
Thanks.

diff --git a/share/functions/export.fish b/share/functions/export.fish
index 7c6248e..5ffd27d 100644
--- a/share/functions/export.fish
+++ b/share/functions/export.fish
@@ -5,7 +5,7 @@ function export --description 'Set global variable. Alias for set -gx, made for
         end
         for arg in $argv
                        # Only split on the first =
-            set -l v (echo $arg | string replace "=" "\n")
+            set -l v (echo $arg | string replace "=" \n)
             set -l c (count $v)
             switch $c
                     case 1
Contributor

lledey commented Oct 12, 2015

There is an issue with this fix, the \n is interpreted as "\n" instead of a line return.
I can't make a pull request but the fix is very simple.
Thanks.

diff --git a/share/functions/export.fish b/share/functions/export.fish
index 7c6248e..5ffd27d 100644
--- a/share/functions/export.fish
+++ b/share/functions/export.fish
@@ -5,7 +5,7 @@ function export --description 'Set global variable. Alias for set -gx, made for
         end
         for arg in $argv
                        # Only split on the first =
-            set -l v (echo $arg | string replace "=" "\n")
+            set -l v (echo $arg | string replace "=" \n)
             set -l c (count $v)
             switch $c
                     case 1
@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Oct 12, 2015

Member

@lledey: Well, that's embarassing. Thanks for pointing it out! Fixed in 54f2152.

Member

faho commented Oct 12, 2015

@lledey: Well, that's embarassing. Thanks for pointing it out! Fixed in 54f2152.

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