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

Adding export command #44

Merged
merged 3 commits into from Feb 29, 2016
Merged

Adding export command #44

merged 3 commits into from Feb 29, 2016

Conversation

nfischer
Copy link
Collaborator

This adds the export command (man page) into the mix.

Similar to how cash $ alias foo ls succeeds (when it wouldn't succeed in bash), cash $ export FOO $PATH also succeeds in redefining FOO to be $PATH.

This exports to process.env, which is case insensitive on Windows (even for the new attributes I'm adding). So, on Windows, you can do things like:

cash $ export FOO='hello'
cash $ echo $foo
hello

I don't think there's any good way to avoid this behavior, so until it's a problem, I suggest it be kept.

This doesn't replace regular assignment. In bash, you can do non-exported assignment of the form: foo=bar, creating a variable named foo with the value "bar". I couldn't figure out a way to write the syntax for regular assignment (posted issue #42 for this).

@dthree do you have any suggestions for additional tests? I modeled unit tests off the ones for alias.

@nfischer
Copy link
Collaborator Author

Also, the contributing page is pretty nicely done 👍

@dthree
Copy link
Owner

dthree commented Feb 29, 2016

Very nicely done!

I don't see anything wrong with this. 👍


Also, the contributing page is pretty nicely done

Oh good - I'm glad it helped. It's hard to keep a balance between to little and too much data that doesn't turn someone off.

dthree added a commit that referenced this pull request Feb 29, 2016
@dthree dthree merged commit f11a47e into dthree:master Feb 29, 2016
@dthree
Copy link
Owner

dthree commented Feb 29, 2016

On a separate note, do we want to publish export as a global module (cash-export)? I don't see why not.


If so, just add it here:

https://github.com/dthree/cash/blob/master/commands.json#L74

It's pretty simple. Just copy paste another and ensure you include all deps. Gulp handles the rest.

@nfischer
Copy link
Collaborator Author

Would it work as a separate module? Internally, it modifies process.env, which gets reset as soon as node exits. Not sure if it would work as its own standalone module.

I suppose it might be useful for export without arguments, since that prints out the contents of process.env.

@dthree
Copy link
Owner

dthree commented Feb 29, 2016

Ah good point.

http://stackoverflow.com/questions/496702/can-a-shell-script-set-environment-variables-of-the-calling-shell

I was thinking that, if called independently, it could export to the current shell's environment. We'd have to find some tricky way to do that.

@nfischer
Copy link
Collaborator Author

I suspect that's not possible (at least on POSIX), since I think it's an operating system security feature. I don't know as much about Windows, however.

@dthree
Copy link
Owner

dthree commented Feb 29, 2016

Okay, let's scratch it.


As the last point on that, we should add export to the global exclusions list:

https://github.com/dthree/cash/blob/master/commands.json#L23

That prevents it from being added to PATH on cash-global.

@nfischer
Copy link
Collaborator Author

I think it should be good (see https://github.com/dthree/cash/blob/master/commands.json#L26). Let me know if I should change something.

@dthree
Copy link
Owner

dthree commented Feb 29, 2016

Oh, silly me, you'd already added it!

Perfect duplication of instructions, btw! Not a single thing wrong on that PR.

@nfischer
Copy link
Collaborator Author

Perfect duplication of instructions, btw! Not a single thing wrong on that PR.

Woohoo! 👍

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.

None yet

2 participants