use named arguments in spec methods

[kamil-gwozdz]

closes #14759

[Blacksmoke16]

So this isn't really forcing these parameters to be named arguments. It's only adding a type restriction to them. You'd really need to do something like this, ditto for the other src/spec/methods.cr files:

Suggested change

	def it(description = "assert", file : String = __FILE__, line : Int32 = __LINE__, end_line : Int32 = __END_LINE__, focus : Bool = false, tags : String | Enumerable(String) | Nil = nil, &block)
	def it(description = "assert", *, file : String = __FILE__, line : Int32 = __LINE__, end_line : Int32 = __END_LINE__, focus : Bool = false, tags : String | Enumerable(String) | Nil = nil, &block)

As called out in https://crystal-lang.org/reference/1.13/syntax_and_semantics/default_values_named_arguments_splats_tuples_and_overloading.html#how-call-arguments-are-matched-to-method-parameters.

I'd be open keeping the more explicit type restrictions if we can ensure they already produce compile time errors if you pass the wrong type as this change would just make it more clear where the problem is without changing the underlying behavior.

EDIT: Might be worth adding new named only overloads and deprecate the current positional ones versus changing things directly to avoid a breaking change as well.

[kamil-gwozdz]

Might be worth adding new named only overloads

Defining it like this isn't possible because external names have to be unique:

# this throws `Error: duplicated def parameter external name: file`:
def it(description = "assert", file : String = __FILE__, *, file file_named = __FILE__) 

do you think it would be okay to rename the positional arguments in this case?

my plan would be to do something like:

def it(description = "assert", file_deprecated : String | Nil = nil, *, file = __FILE__)
  file = file_deprecated || file 
  raise_deprecation_warning unless file_deprecated.nil?

WDYT? Would it be ok to execute the deprecation warning check at runtime? Or does it have to happen at compilation time? I found this code that handles deprecation logic but it doesn't seem to handle the kind of the change that I want to make.

[kamil-gwozdz]

nvm, i just figured out that it's possible to overload the whole method

[Blacksmoke16]

They would need to be separate overloads, versus just messing with the existing method. We definitely want the warnings to be at compile time.

Something along the lines of:

def it(description : String = "assert", *, file : String = __FILE__, line : Int32 = __LINE__, end_line : Int32 = __END_LINE__, focus : Bool = false, tags : String | Enumerable(String) | Nil = nil, &block)
  Spec.cli.root_context.it(description.to_s, file, line, end_line, focus, tags, &block)
end

# :ditto:
@[Deprecated("Arguments other than the first should be provided as named arguments.")]
def it(description = "assert", _file file = __FILE__, _line line = __LINE__, _end_line end_line = __END_LINE__, _focus focus : Bool = false, _tags tags : String | Enumerable(String) | Nil = nil, &block)
  Spec.cli.root_context.it(description.to_s, file, line, end_line, focus, tags, &block)
end

However it seems like compiler doesn't really like this as:

require "spec"

it "foo", line: 123 do
  1.should eq 1
end

it "bar" do
  2.should eq 2
end

Results in an unexpected:

In test.cr:7:1

 7 | it "bar" do
     ^-
Warning: Deprecated ::it. Arguments other than the first should be provided as named arguments.

A total of 1 warnings were found.

It does work as expect with the -Dpreview_overload_order flag, but idt we can rely upon that until Crystal 2.0. So not sure what our options are at this point...

[Blacksmoke16]

I figured out a way for this to work I think. It's a bit less than ideal, but does allow using the @[Deprecated] annotation, with both versions showing up in the API docs.

The problem with it picking the correct overload stems from the fact the methods are defined in a module then included into the top level namespace. What we can do to work around that is add an empty bodied overload with the proper type restrictions and :ditto: doc comment. This will allow both overloads to show up in the API docs, with the old one flagged as deprecated.

Then we can put the real implementations of the overloads in a macro included so that they are also added to the top level, but after the ones from the module itself. From my toy example I don't get any deprecation warnings so it seems to be working at least.

  # ... 
  #
  # <Docs for the current method>
  @[Deprecated("Arguments other than the first should be provided as named arguments.")]
  def it(_description description = "assert", _file file = __FILE__, _line line = __LINE__, _end_line end_line = __END_LINE__, _focus focus : Bool = false, _tags tags : String | Enumerable(String) | Nil = nil, &block)
    Spec.cli.root_context.it(description.to_s, file, line, end_line, focus, tags, &block)
  end

  # :ditto:
  def it(description : _ = "assert", *, file : String = __FILE__, line : Int32 = __LINE__, end_line : Int32 = __END_LINE__, focus : Bool = false, tags : String | Enumerable(String) | Nil = nil, &block)
  end

  # ...

  macro included
    def it(description : _ = "assert", *, file : String = __FILE__, line : Int32 = __LINE__, end_line : Int32 = __END_LINE__, focus : Bool = false, tags : String | Enumerable(String) | Nil = nil, &block)
      Spec.cli.root_context.it(description.to_s, file, line, end_line, focus, tags, &block)
    end

    # ...
  end

WDYT?

EDIT: I think it's impt to not add an explicit String restriction on description. As that would prevent the very common use case of doing things like describe MyClass. Maybe for now we at least make it explicit via a _ type restriction?

[straight-shoota]

suggestion: We could use this opportunity to introduce type restrictions for all parameters (as previsously mentioned in
#14915 (comment)).