Skip to content
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

Fixes jamesob/desk#56 adding support for displaying exported environment variables in deskfiles #60

Merged
merged 7 commits into from
Mar 19, 2016

Conversation

cfont
Copy link
Contributor

@cfont cfont commented Mar 10, 2016

I've added some working code to display the exported environment variables defined in a desk file as requested in #56 by @rafaelbco. These code changes do compile and pass tests but that is probably because the tests haven't been increased to include whether this works or not ;-) but I guess the good thing is that it doesn't break anything else.

Please review and let me know if this is what you would like and I'll work on adding tests for this specific use case.

@rafaelbco
Copy link

Works as expected for me.

I suggest one improvement: add the $ character before the variable name in the output, like this:

$MYVAR  My environment variable.

Thanks a lot @cfont !

@jamesob
Copy link
Owner

jamesob commented Mar 10, 2016

Sweet! Nice work, @cfont!

Just want to be sure we package some tests with this. I'll write some a bit later unless you wanna take a crack at it yourself -- probably somewhere in here?

@cfont
Copy link
Contributor Author

cfont commented Mar 10, 2016

I want to take a crack at it myself and am planning to add it below the tests for hi and howdy which means I'll also need to modify that example hello deskfile to have one set. Between tonight and tomorrow, I plan on taking time to do some work on this.

This commit includes two file updates for testing the new feature
as requested in jamesob#56. The example hello deskfile now has
a variable set called MyName with a value "James" and the runtests
shell script includes a test to echo the value of that variable.

This test proves that the variable is exported as expected from the
deskfile and is retrievable as expected.

Still #todo includes a test that lists the description of the deskfile
and looks for that variable to be listed along with the aliases and
functions.
I've added a test to make sure the MyName variable in the newly modified
hello Deskfile is listed in the output when just issuing the desk command
(list current desk and any associated aliases)
@cfont
Copy link
Contributor Author

cfont commented Mar 16, 2016

Ok, so I've got a couple of new tests in here and an updated hello.sh deskfile for exported variables being displayed in the "What desk am I using currently" output. Everything is working and ready for your reviews and comments.

@rafaelbco, how important is it at this moment to have the $ character in front of the variable name? I ask because it isn't as "easy" as just adding it there since the code currently cycles through all of the "things" in the deskfile creating an array and then outputting them without regard to what kind of "thing" it might be. In other words, it currently doesn't discern whether it is an alias, function, or variable when rendering the output. If I go through and figure out which ones might be variables to prepend the $ character then I feel like it may be worth going ahead and separating them all by sections. Something like you originally used in your example:

mydesk - my desk to work on things.

  Aliases:
  howdy    Use if you're from Texas

  Functions:
  hi       Say in informal hello

  Environment variables:
  $MyName  James

My questions to everyone are:

  • do you want that indicator in this PR?
  • if so, do you also want some extra grouping?

@rafaelbco
Copy link

Well, for me the $ indicator is not that important. I think it's better to
keep this PR simple.

Rafael Oliveira

On Wed, Mar 16, 2016 at 5:26 PM, Chris Fontenot notifications@github.com
wrote:

Ok, so I've got a couple of new tests in here and an updated hello.sh
deskfile for exported variables being displayed in the "What desk am I
using currently" output. Everything is working and ready for your reviews
and comments.

@rafaelbco https://github.com/rafaelbco, how important is it at this
moment to have the $ character in front of the variable name? I ask because
it isn't as "easy" as just adding it there since the code currently cycles
through all of the "things" in the deskfile creating an array and then
outputting them without regard to what kind of "thing" it might be. In
other words, it currently doesn't discern whether it is an alias, function,
or variable when rendering the output. If I go through and figure out which
ones might be variables to prepend the $ character then I feel like it may
be worth going ahead and separating them all by sections. Something like
you originally used in your example:

mydesk - my desk to work on things.

Aliases:
howdy Use if you're from Texas

Functions:
hi Say in informal hello

Environment variables:
$MyName James

My questions to everyone are:

  • do you want that indicator in this PR?
  • if so, do you also want some extra grouping?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#60 (comment)

@jamesob
Copy link
Owner

jamesob commented Mar 19, 2016

@cfont @rafaelbco thanks, guys! Nice work with the tests, @cfont.

jamesob added a commit that referenced this pull request Mar 19, 2016
Fixes #56 adding support for displaying exported environment variables in deskfiles
@jamesob jamesob merged commit 4cab6dd into jamesob:master Mar 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants