-
-
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
Refactor spec runner CLI for extensibility #5763
Refactor spec runner CLI for extensibility #5763
Conversation
Introduce `Spec::CLI` class as new entry point of Spec runner. This new interface makes more easy to extend runner's behavior and options by extensions or formatters, overriding `#setup` and using previous definition: # TAP formatter extension (spec/tap.cr) require "spec/cli" class Spec::CLI def setup # important: include previous definition! previous_def # extend with new option parser.on("--tap", "use my own TAP formatter") do options.default_formatter = Spec::TAPFormatter.new end end end # my spec_helper.cr require "spec" require "spec/tap" With these changes will be possible to extend Spec runner with other formatters or extensions. It provides 1:1 behavior found in `spec.cr`, but it can be improved more easily moving forward (ie. register formatters, etc.) Ref #5662
This is exactly the feature I was thinking about yesterday 😃 (spec's CLI extending) Nice PR! |
spec/std/spec/cli_spec.cr
Outdated
cli.exited.should eq(0) | ||
|
||
io.rewind | ||
output = io.gets_to_end |
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.
Instead of:
io.rewind
output = io.gets_to_end
You can do: output = io.to_s
(and ditto everywhere you use this)
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.
Completely forgot about that, will update. Thank you.
Shorter than rewind and gets_to_end.
src/spec/cli.cr
Outdated
property formatters = Array(Spec::Formatter).new | ||
property locations = Array({String, Int32}).new | ||
|
||
property! default_formatter : Spec::Formatter |
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.
Why don't you just provide default values or make the properties nilable? You should avoid running into unmet nil assertions which could very easily happen if someone just calls options.default_formatter
when the property is not set.
You can use property?
for nilable values.
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.
I didn't want to introduce default values here, at least not on this refactor. I tried to bring things as close as possible to the original to reduce the PR size and the changes to be reviewed.
Definitely this could be improved moving forward (I already have another draft to improve formatter registration and remove junit-specific code).
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.
I got why you did it this way, but then please make the properties nilable because they are.
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.
That will force me to change the code in apply_options
and use local variables instead.
From this:
Spec.pattern = options.pattern if options.pattern?
To this:
if pattern = options.pattern?
Spec.pattern = pattern
end
Perhaps I'm missing something in the approach you're suggesting, so will appreciate if you can provide a code example with the reasoning instead.
Thank you.
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.
Well, the reasoning is that if you happend to call options.default_formatter
(maybe from a tool that interfaces with this spec CLI) you wouldn't be aware that calling this simple accessor might raise a runtime error. This should be avoided. property!
should only be used for properties that are usually expected to not be nil when the object is used (for example after setting up) and accessing them in the intended way will not raise a nil assertion.
That's not the case here. Each of these options can either have a value of a specific type or Nil
. That nilable should be expressed in the code and handled appropriately. Your current implementation tries to treat the options as not being nilable.
As for the usage: assigning to a local variable is the proper Crystal way to handle nilable instance variables. I don't see why you would want to avoid that in favour of easily breakable code.
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.
Thank you for expressing the reasoning behind your suggestions. Will push changes to correct usage of property.
Cheers.
|
||
getter options : Options | ||
|
||
private getter argv : Array(String) |
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.
private getter
is a bit useless - why not just access the ivars directly (@argv
)?
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.
I disagree on the usefulness of it. private getters are used across the codebase and internals in similar way.
In the case of argv
, not using it directly since it will require me to assign to local before I can retrieve .first
(see #prepare
).
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.
it will require me to assign to local before I can retrieve .first
why?
Also I really don't care use whichever style you like.
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.
Why is arv
stored as an ivar anyway? As far as I can see, it's only evaluated in the prepare
method. So having Spec::CLI.new.run(ARGV)
and (prepare(arv)
) would be a better API and remove the need to store this.
src/spec/cli.cr
Outdated
end | ||
|
||
def run | ||
prepare unless prepared? |
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.
Don't need the condition here, will be checked in the method anyway.
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.
Ah, was a left over from previous tests. Will remove. Thank you.
spec/std/spec/cli_spec.cr
Outdated
|
||
# override and capture exit calls | ||
module Spec | ||
class CLI |
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 could be a subclass of Spec::CLI
which is only used for testing and has appropriate overwrites.
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.
Ah, didn't thought that way. Care to suggest a name for such class?
Thank you.
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.
class TestCLI < Spec::CLI
?
This would be useful for #5671 as well. |
Inherit and adjust instead, simplifying code and avoding conditionals
I was incorrectly using `.not_nil!` for certain options that were by default nilable. Updated to use `property?` instead and adjusted accordingly.
I've pushed additional changes based on the feedback provided. I would prefer to squash the commits before merging (once this gets approved). Cheers. |
src/spec/cli.cr
Outdated
property formatters = Array(Spec::Formatter).new | ||
property locations = Array({String, Int32}).new | ||
|
||
property? default_formatter : Spec::Formatter? = nil |
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.
Lil' bit of nitpicking:
- No need for nilable postfix
?
, since it's added automatically by theproperty?
macro - Default
nil
assignments are redundant - All properties except
Bool
IMHO should use normalproperty
macro
ditto all below
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.
No need for nilable postfix ?, since it's added automatically by the property? macro
I'm not sure that statement is correct. When property?
macro expands, does not define a union:
class Foo
property? something : Bool
end
$ ccr tool expand -c 1.cr:2:3 1.cr
Using compiled compiler at `.build/crystal'
Error in 1.cr:2: expanding macro
property? something : Bool
^
in macro 'property?' expanded macro: macro_94853763181520:797, line 3:
1.
2.
> 3. @something : Bool
4.
5. def something? : Bool
6. @something
7. end
8.
9. def something=(@something : Bool)
10. end
11.
12.
13.
instance variable '@something' of Foo was not initialized directly in all of the 'initialize' methods, rendering it nilable. Indirect initialization is not supported.
Default nil assignments are redundant
Agree on that since Type?
will default to nil
. Will remove
All properties except Bool IMHO should use normal property macro
I don't think so. Usage of ?
-style for the properties is not about boolean value, but the Crystal pattern to check for nil
. This follows the pattern of Hash#[]?
, fetch?
, first?
and many others.
Perhaps you refer to something else? Please explain if possible with a concrete example so I can better understand your comment.
Thank you.
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.
I'm not sure that statement is correct. When property? macro expands, does not define a union:
My bad, it's the case only for property!/getter!
macros.
I don't think so. Usage of ?-style for the properties is not about boolean value, but the Crystal pattern to check for nil. This follows the pattern of Hash#[]?, fetch?, first? and many others.
?
-style getters are mostly used alongside of their ?
-less counterparts. Normal attributes tends to be defined as either of those, with Exception#message
which return type is String?
is one (counter-)example that comes to my mind.
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.
Thank you for your comments. I guess will wait for input from Crystal core in relation to the style used, as this is not outlined in Conventions / Coding style documentation.
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.
I'd say it is quite clear: Indicating a nilable property type with ?
-suffix is not common practice. A method with ?
-suffix makes only sense as an alternative to a method which would raise instead of returning nil
. Another use case are predicate methods returning Bool
.
All uses of the property?
macro in the standard library are with a Bool
type. Nilable properties always use property
.
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.
Good point, didn't explore the entire codebase back then to extrapolate that pattern, but I have done that last night.
Will still wait for Crystal core feedback prior sending further commits, as I have quite limited OSS time this week.
Cheers.
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.
?
suffix is typically either asking a question or a nillable version of a method without a ?
. I wouldn't write it like this.
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.
I wouldn't write it like this.
Can you be more specific? You wouldn't write it like shown in current code? you wouldn't change it to be non-?
, or by the given examples in this conversation?
Please note that not everybody here is a native English speaker. Sometimes English is not even our second language. The more precise you can be with your statements the better we will benefit from your feedback.
Thank you.
Unions with Nil defaults to nil.
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.
I don't see the point in the Options
class, you just twiddle bits in a class just to copy the exact same settings to Spec
.
In fact I don't see much point in this whole PR - what are the usecases for monkey-patching spec options parsing?
src/spec/cli.cr
Outdated
property formatters = Array(Spec::Formatter).new | ||
property locations = Array({String, Int32}).new | ||
|
||
property? default_formatter : Spec::Formatter? = nil |
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.
?
suffix is typically either asking a question or a nillable version of a method without a ?
. I wouldn't write it like this.
|
||
getter options : Options | ||
|
||
private getter argv : Array(String) |
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.
it will require me to assign to local before I can retrieve .first
why?
Also I really don't care use whichever style you like.
src/spec/cli.cr
Outdated
options.default_formatter = Spec::VerboseFormatter.new | ||
end | ||
ensure | ||
@prepared = true |
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.
But you haven't prepared if there's an exception.
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.
Good point, will correct.
The introduction of the
I can list a few cases:
The whole idea here is to avoid monkey patching and offer a framework to others introduce additional changes without having to wait an entire release cycle. This is the purpose of this change, as mentioned in the PR description, relates to #5662 and other issues/PR associated. Apologies if the whole purpose of this PR wasn't clear from the description, hope the above provides the proper background. Cheers. |
That's the problem. I personally think that should come before or in this PR.
I'd rather have the |
I have to disagree with that. In my personal experience and monitoring PR requests and process at Crystal, there is a tendency for huge PR to age without review for long time. I would rather get smaller, digestible-size PRs that progressively drive us to that objective than a single, 400+ lines PR that sits for months in the pool without nobody looking at it. Of course, there are downsides like in this case: a smaller PR that doesn't fulfill everybody's expectations. There isn't a perfect PR, like there isn't a perfect language, library, etc.
That is great to hear, and perhaps will be great if your ideas get drafted somewhere, like a RFC issue where others can weight in and help design a solution for this. In the meantime, this is my little contribution towards a better outcome. Cheers. |
Keep SpecRunnerCLI private. Also make sure `exit_code` is not modified externally.
I'd suggest to make this PR a little bit smaller and remove the Also, I'm not sure if the class should be called I'll point out a few simplifications in the line comments. |
|
||
getter options : Options | ||
|
||
private getter argv : Array(String) |
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.
Why is arv
stored as an ivar anyway? As far as I can see, it's only evaluated in the prepare
method. So having Spec::CLI.new.run(ARGV)
and (prepare(arv)
) would be a better API and remove the need to store this.
end | ||
|
||
it "aborts on incorrect location format" do | ||
io = IO::Memory.new |
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.
It's better to use String.build do |io|
getter options : Options | ||
|
||
private getter argv : Array(String) | ||
private getter parser = OptionParser.new |
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.
Again, why does this need to be stored in an ivar? It is just needed in prepare
. You could simply initialize it there and call setup(parser)
(maybe renamed to setup_parser(parser)
).
private class SpecRunnerCLI < Spec::CLI | ||
getter exit_code : Int32? | ||
|
||
private def display(message) |
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 mock shouldn't override display
and terminate
but exit
. If you define it as a private instance method, it shadows the top level ::exit
.
Thank you @straight-shoota for the feedback and the willing to get this moving forward. I'm a bit limited on time the upcoming days but might take a look soon and even better send a proper RFC on some of the architecture changes that will make the work on this PR obsolete. Cheers. |
Introduce
Spec::CLI
class as new entry point of Spec runner. This new interface makes more easy to extend runner's behavior and options by extensions or formatters, overriding#setup
and using previous definition:With these changes will be possible to extend Spec runner with other formatters or extensions.
It provides 1:1 behavior found in
spec.cr
, but it can be improved more easily moving forward (ie. register formatters, etc.)Ref. #5662