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

Forwarding command line arguments to raco test #1726

Merged
merged 14 commits into from
Jun 22, 2017
Merged

Conversation

cfinegan
Copy link
Contributor

The changes in this pull request allow users of raco test to specify additional command-line arguments to be forwarded to any invoked test modules. This enables authors of testing libraries to use these forwarded arguments as a set of predicates upon which to filter the running tests, or any other purpose they see fit.

The forwarded arguments are specified with a trailing -- after all flags and argument paths, followed by any arguments to be forwarded. For example, the command raco test module.rkt -- more args will invoke module.rkt with the command-line arguments more args. These additional arguments are combined with any arguments specified in info.rkt by test-command-line-arguments.

@mflatt
Copy link
Member

mflatt commented Jun 19, 2017

Thanks for the PR! See also #1357. I think @samth didn't merge that one because he didn't finish the docs, so I appreciate that you've added docs here.

The ++args approach used in #1357 is more in line with the way other Racket tools pass along command-line arguments. The -- approach here seems more convenient, but it's not as composable, because there's no way to point raco test at a file whose name is -- – which maybe doesn't seem like the sort of thing anyone would want to do with raco test, but that's more an explanation of why other tools use ++args, and we'd like to make things as consistent as possible.

If you're not opposed to the ++args approach, I wonder if you'd be willing to merge the PRs. If you really want --, we can discuss more.

@mflatt
Copy link
Member

mflatt commented Jun 19, 2017

P.S. I think ++args should have been ++arg.

@cfinegan
Copy link
Contributor Author

I don't have a strong opinion on the syntax, the -- method was proposed by @jeapostrophe so I based my implementation on that. I'd be more than happy to to merge this request with #1357 if a consensus can be reached on the syntax. It seems like my implementation breaks some of the automated tests, so keeping the -- syntax would require more work than using ++arg.

The one possible issue I can see with ++arg is: Would it allow the user to specify multiple command-line arguments to be forwarded easily? Or would it require one to type ++arg <arg> once for each arg?

@mflatt
Copy link
Member

mflatt commented Jun 19, 2017

The current convention is that ++arg would add only one argument at a time, which is why I suggest ++arg instead of ++args.

I can imagine a ++args that adds multiple space-separated arguments at once. Space-separated arguments within a flag argument would be a new convention, as far as I can remember, but it doesn't conflict with any existing convention or composition possibilities as long as ++arg also exists.

@jeapostrophe
Copy link
Collaborator

Thanks for the feedback @mflatt I think that ++arg and ++args are the right thing for Conor to do. Sorry for not noticing it before.

@cfinegan
Copy link
Contributor Author

I'm having some issues getting ++args to work correctly. It doesn't seem like command-line has any support for flags with variable arguments, and I don't think it's viable to hack in this functionality. The issue I'm seeing is that I don't think there's anything that immediately and obviously distinguishes between an argument to a flag and an argument to the test framework itself.

For example, if the user types raco test ++args foo bar -s mod file, then it's obvious that foo and bar are arguments to ++args, whereas mod is an argument to -s and file is the name of file/directory to test.

But consider if the user types raco test ++args foo bar file. It's much less obvious what the types of the arguments are. Are foo and bar both arguments to ++args? Or are bar and file both file/directory arguments to test? I don't see any clear path forward in terms of getting these features to play nice with each other.

@mflatt
Copy link
Member

mflatt commented Jun 20, 2017

You're right that raco test ++args foo bar file can't work. A user would have to write raco test ++args "foo bar" -s mod file (in practically any shell) to make foo and bar a single argument of ++args, and then ++args must parse out the separate arguments itself.

What if a user wants to pass hello world and something else as two different arguments? I suggest that you don't try to handle that with ++args, and the user would have to go with ++arg "hello world" ++arg "something else", instead.

In other words, ++args will have its own little parsing sublanguage, which doesn't need to be as expressive as a shell.

@cfinegan
Copy link
Contributor Author

I see, thank you!

@cfinegan
Copy link
Contributor Author

Okay ++args should be ready for prime time now, unless anyone has any issue with the wording in the docs.

@@ -91,9 +93,9 @@
(provide go)
(define (go pch)
(define l (place-channel-get pch))
(define args (cadddr (cdr l)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you use list-ref instead of the old school way?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Eh, it looks like you are just adapting old code so no worries

@jeapostrophe
Copy link
Collaborator

I think this should be merged

@mflatt
Copy link
Member

mflatt commented Jun 22, 2017

This looks good to me, too, so I'll merge.

I'll probably adjust the doc wording "must be enclosed in quotation marks", because that assumes a particular kind of shell context, but the intent is clear enough, and I'll have to give the right words more thought.

@jeapostrophe
Copy link
Collaborator

Excellent

@mflatt mflatt merged commit 9852afe into racket:master Jun 22, 2017
@mflatt
Copy link
Member

mflatt commented Jun 22, 2017

We forgot about adding a history note to the docs, so I'll do that, too.

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

4 participants