-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add usage instructions for spec runner to compiler #10046
Add usage instructions for spec runner to compiler #10046
Conversation
989e15d
to
efd4a3c
Compare
This is awesome! I had a very similar refactoring locally (which included a lot more than just the CLI), so I'm very happy to see this. |
src/compiler/crystal/command/spec.cr
Outdated
class Crystal::Command | ||
private def spec | ||
compiler = new_compiler | ||
link_flags = [] of String | ||
OptionParser.parse(options) do |opts| | ||
opts.banner = "Usage: crystal spec [options] [files]\n\nOptions:" | ||
opts.banner = "Usage: crystal spec [options] [files] [-- runtime_options]\n\nOptions:" |
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.
The runtime_options can be passed before the file, there is no need to use --
to split them. Wouldn't that be the preferred way to advertise them?
In that line of thought I would call the "Spec runner additional options" or somehing like that.
I find it confusing that they are presented as "Runtime options"
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.
Yes, the --
can be removed. I'd still leave it as [options] [files] [runtime_options]
even though they can be mixed. Don't think there's a better way to do this and this makes at least sense from a structural PoV.
Naming is particularly hard with this one. I'm not convinced Spec runner additional options
improves it, though. I would have more questions about that than Runtime options
which is really just a factual description. Those are the options evaluated at runtime by the spec runner, the others are compiler options.
I would love to see this in 1.0 as it's a really nice and cheap enhancement for developer convenience. |
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.
Wow thank you @straight-shoota, this is great developer UX 💯 I'd also love to see this land in 1.0 if possible 🤔
Currently,
crystal spec --help
only shows compile time options. The runtime options are missing and only show up once you have compiled a spec runner.This separation has technical reasons but results in aweful user experience.
crystal spec
presents itself as an holistic interface for running specs and transparently delegates runtime options to the executable. You can pass runtime options directly tocrystal spec
, but they don't show up in the usage overview. So many of the options are hidden to the user. You only discover them if you are aware of runningcrystal spec -- --help
.This is the old (separate) output:
With this patch the usage instructions for the spec runner are included in the usage instractions for the compiler spec command.
The actual
OptionParser
from stdlib is used for that to avoid maintaining different instructions and have them slip out of sync. There would only be a discrepancy when the compiled stdlib version has different options than the one the compiler was built with. But that's acceptable.The only option to avoid that would be to compile or somehow extract the information from
Spec
module from stdlib source. This could be more feasible if a textual representation like docopt was used instead ofOptionParser
builing it at runtime.In order to not include large parts of the
Spec
module in the compiler, the option parser configuration has been extracted tosrc/spec/cli.cr
in a way that it does not rely on any other spec feature. The only intricacy is setting up the formatters. I've introducedSpec.configure_formatter
for this which has a blank implementation incli.cr
and the real one inspec.cr
overrides it for actual use.New output of
crystal spec --help
: