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

Only use unix-style variable references #38

Merged
merged 4 commits into from
Feb 28, 2016
Merged

Only use unix-style variable references #38

merged 4 commits into from
Feb 28, 2016

Conversation

nfischer
Copy link
Collaborator

This is different than the direction that was decided on in #34 (see comment), but I think this might work out better in the long run (for posix compatibility). I opted to make this a separate pull request (rather than tack on to #37) since this is a very different approach.

This means things like %PATH% always evaluate to the literal %PATH%, while references of the style $PATH or ${PATH} evaluate to the value of the variable (or the empty string if this variable doesn't exist).

The main advantages of this are:

  1. It's (mostly) consistent with POSIX (so echo %path% is guaranteed to be the literal value in both cash and bash)
  2. This is a shorter implementation, and (IMO) easier to understand
  3. It probably performs slightly better on Unix systems (I haven't tested this, so I can't confirm)
  4. Case sensitive variable names for Unix, case insensitive names for Windows (so either $PATH or $path work on Windows)

This means that really tricky windows-specific examples (like echo hi%invalid%computername%username%username%%%) aren't really going to have support (I can't think of an equivalent bash syntax that would have the same semantics). So, this is a tradeoff, but I think it's one that's probably worth discussing. And if POSIX compatibility is the goal, you probably won't need to support the other behavior.

@dthree
Copy link
Owner

dthree commented Feb 28, 2016

Okay, yeah, let's just do Linux. I'm okay with this. Not for code simplicity, but for compatibility.


  1. If we're going full-on bash, just drop the support for case sensitivity.
  2. Broken AppVeyor test.

After that, ready to merge!

if (windows) {
cash('echo %PATH%-%PATH%').should.equal(`${path}-${path}\n`);
// Case insensitive
Copy link
Owner

Choose a reason for hiding this comment

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

If we're going full-on bash, just drop the support for case sensitivity.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, the issue here is that I put %path% instead of $path (I forgot to change it). Fixed this, and I'll push again.

Copy link
Owner

Choose a reason for hiding this comment

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

Perfect.

@dthree
Copy link
Owner

dthree commented Feb 28, 2016

Btw, I know you've got ShellJS all over your plate, but I thought I'd ask if you'd like to be a collaborator on Cash. It'd be good to have someone with experience on such a similar project.

@nfischer
Copy link
Collaborator Author

I'm more than willing to help out where I can. I have a few projects I'm working on right now, so I probably won't be too helpful with reviewing or making PRs, but I can chime in on POSIX compatibility issues when they arise.

@dthree
Copy link
Owner

dthree commented Feb 28, 2016

Okay that would be great. Thanks!

@nfischer
Copy link
Collaborator Author

@dthree Should be good now. I'm not sure why coveralls is failing (I don't have experience with it, personally)

@dthree
Copy link
Owner

dthree commented Feb 28, 2016

I think it just "fails" whenever you don't up coverage. Kind of stupid.

dthree added a commit that referenced this pull request Feb 28, 2016
Only use unix-style variable references
@dthree dthree merged commit 8b2d45c into dthree:master Feb 28, 2016
@dthree
Copy link
Owner

dthree commented Feb 28, 2016

Looks great, you're awesome. 👍

@nfischer
Copy link
Collaborator Author

👍

@nfischer nfischer mentioned this pull request Feb 28, 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.

None yet

2 participants