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
don't return the command name in list of arguments #12
don't return the command name in list of arguments #12
Conversation
9bb0d42
to
f4d1a41
Compare
Hi, what is the status of this PR? I don't mind if you don't want to incorporate it for concern of breaking existing users. What do you think? |
Hello @armcknight, Sorry for not replying earlier I kind of miss setup the notifications. I will check this over the weekend :). |
@@ -25,7 +25,8 @@ class ArgsTests: QuickSpec { | |||
"no-ff": ""] | |||
|
|||
expect(Args.parsed.flags).to(equal(result)) | |||
expect(Args.parsed.parameters).to(equal(["Some custom one", "one", "two"])) | |||
expect(Args.parsed.parameters).to(equal(["one", "two"])) | |||
expect(Args.parsed.command).to(equal("Some custom one")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This expectation here is a little misleading, I think we should refactor this test.
we can set the process info above to be
ProcessInfo.internalProcessInfo = DummyProcessInfo("excutable_name", "-f", "file.rb", "--integer", "1", "Some custom one", "one", "two", "--no-ff")
and then edit this expectation to be
expect(Args.parsed.command).to(equal("excutable_name"))
This looks super cool! I have added one note on the unit test. |
f4d1a41
to
153e6b6
Compare
- fix unit test mock and expectations in ArgsTests
153e6b6
to
7955f11
Compare
Hey @oarrabi , thanks for the comments, I updated all the things requested! |
This is awesome! |
…ent-list don't return the command name in list of arguments
👍 Thanks for the library, enjoying using it! |
This makes the behavior line up with description in the README