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

Alias test rework #1124

Merged
merged 2 commits into from
Nov 11, 2016
Merged

Alias test rework #1124

merged 2 commits into from
Nov 11, 2016

Conversation

mpeck
Copy link
Contributor

@mpeck mpeck commented Nov 4, 2016

Reworks alias command tests. This also includes work to the which command tests since they are tied closely to aliases.

part of #1122

@mpeck mpeck added the review label Nov 4, 2016
Copy link
Member

@kevsmith kevsmith left a comment

Choose a reason for hiding this comment

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

I didn't see any tests for aliases containing variable references. We should test that case in light of #1135

@@ -14,7 +14,7 @@ defmodule Cog.Commands.Alias.Create do
-h, --help Display this usage info

EXAMPLE
alias create my-awesome-alias "echo \"My awesome alias\""
alias create my-awesome-alias \"echo \"My awesome alias\"\"
Copy link
Member

Choose a reason for hiding this comment

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

I don't think escaping those quotes are necessary here, although I think Elixir renders them just the same inside a triple quote block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually added that because my syntax highlighter was getting mucked up and it was difficult to tell what was going on. I meant to remove it. My bad.

@mpeck
Copy link
Contributor Author

mpeck commented Nov 9, 2016

@kevsmith there is actually another set of tests that test alias expansion and execution, that's actually where I ran across the bug addressed in #1135. This is just to test the alias command itself.

Copy link
Member

@kevsmith kevsmith left a comment

Choose a reason for hiding this comment

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

I'm not clear on the distinction between alias command tests and tests targeting alias creation (which is accomplished with the alias command) but let's get this merged.

@mpeck mpeck merged commit 96ed097 into master Nov 11, 2016
@mpeck mpeck removed the review label Nov 11, 2016
@mpeck mpeck deleted the peck/alias-tests branch November 11, 2016 20:25
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