Instead of:

io.rewind
output = io.gets_to_end

You can do: output = io.to_s

(and ditto everywhere you use this)

Completely forgot about that, will update. Thank you.

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.

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).

I got why you did it this way, but then please make the properties nilable because they are.

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.

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.

Thank you for expressing the reasoning behind your suggestions. Will push changes to correct usage of property.

Cheers.

private getter is a bit useless - why not just access the ivars directly (@argv)?

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).

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.

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.

Don't need the condition here, will be checked in the method anyway.

Ah, was a left over from previous tests. Will remove. Thank you.

This could be a subclass of Spec::CLI which is only used for testing and has appropriate overwrites.

Ah, didn't thought that way. Care to suggest a name for such class?

Thank you.

class TestCLI < Spec::CLI?

Lil' bit of nitpicking:

• No need for nilable postfix ?, since it's added automatically by the property? macro
• Default nil assignments are redundant
• All properties except Bool IMHO should use normal property macro

ditto all below

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.

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.

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.

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.

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.

? suffix is typically either asking a question or a nillable version of a method without a ?. I wouldn't write it like this.

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.

? suffix is typically either asking a question or a nillable version of a method without a ?. I wouldn't write it like this.

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.

But you haven't prepared if there's an exception.

Good point, will correct.

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.

It's better to use String.build do |io|

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)).

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.