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

Handle @option tags #59

Open
connorshea opened this issue Jun 30, 2019 · 17 comments
Open

Handle @option tags #59

connorshea opened this issue Jun 30, 2019 · 17 comments
Labels
enhancement New feature or request

Comments

@connorshea
Copy link
Contributor

Is your feature request related to a problem? Please describe.
Selenium's Ruby gem (it's in the rb/ directory) has YARD docs for an options hash:

# Create a new Wait instance
#
# @param [Hash] opts Options for this instance
# @option opts [Numeric] :timeout (5) Seconds to wait before timing out.
# @option opts [Numeric] :interval (0.2) Seconds to sleep between polls.
# @option opts [String] :message Exception mesage if timed out.
# @option opts [Array, Exception] :ignore Exceptions to ignore while polling (default: Error::NoSuchElementError)
def initialize(opts = {})
  @timeout  = opts.fetch(:timeout, DEFAULT_TIMEOUT)
  @interval = opts.fetch(:interval, DEFAULT_INTERVAL)
  @message  = opts[:message]
  @ignored  = Array(opts[:ignore] || Error::NoSuchElementError)
end

Code here

But Sord doesn't handle this correctly.

class Wait
  sig { params(opts: Hash).returns(Wait) }
  def initialize(opts = {}); end

Describe the solution you'd like
I'm not 100% sure whether Sorbet has a good way to output the exact attributes of a hash inline, but it'd be nice if we could generate proper type information from this kind of YARD doc.

Additional context

@connorshea connorshea added the enhancement New feature or request label Jun 30, 2019
@connorshea
Copy link
Contributor Author

Excluding YARD (it has some false positives in specs), the gems in the sord_examples/ directory have a total of 680 instances of # @option.

@connorshea
Copy link
Contributor Author

Also worth noting that the docs don't need to include the opts hash as a param, as mentioned in the YARD docs.

For example in rouge:

# Guess which lexer to use based on a hash of info.
#
# @option info :mimetype
#   A mimetype to guess by
# @option info :filename
#   A filename to guess by
# @option info :source
#   The source itself, which, if guessing by mimetype or filename
#   fails, will be searched for shebangs, <!DOCTYPE ...> tags, and
#   other hints.
# @param [Proc] fallback called if multiple lexers are detected.
#   If omitted, Guesser::Ambiguous is raised.
#
# @see Lexer.detect?
# @see Lexer.guesses
# @return [Class<Rouge::Lexer>]
def guess(info={}, &fallback)
  lexers = guesses(info)

  return Lexers::PlainText if lexers.empty?
  return lexers[0] if lexers.size == 1

  if fallback
    fallback.call(lexers)
  else
    raise Guesser::Ambiguous.new(lexers)
  end
end

This is still valid.

@connorshea
Copy link
Contributor Author

I guess Typed Structs are what should be used here?

https://sorbet.org/docs/tstruct

@AaronC81
Copy link
Owner

AaronC81 commented Jun 30, 2019

Typed structs seem to suit this best, but from the docs I can't see how you can model hashes with them, only structs. Is there an example of using them with hashes anywhere?

Also, if I understand correctly, the typed structs would actually need a name. How about, for a method def foo_bar_baz(x={}), its @options typed struct could be named FooBarBazX?

@connorshea
Copy link
Contributor Author

That's a good question, I'm not sure of any examples of using Typed Structs for hashes, I'll look around.

And yeah they'd need a name, your solution sounds good enough to me.

@connorshea
Copy link
Contributor Author

connorshea commented Jun 30, 2019

This seems to work:

# typed: strict
require 'sorbet-runtime'

class MyStruct < T::Struct
  prop :x, Integer, default: 1
end

my_first_struct = MyStruct.new({x: 2})

my_second_struct = MyStruct.new
my_second_struct.x = 1

sig { params(options: MyStruct).void }
def method(options)
  puts options.x
end

method(my_first_struct)
method(my_second_struct)

Although, the file won't run due to sig not being defined 🤔

EDIT: Needed to add extend T::Sig, though now I just have a new error :D

EDIT 2: Oh I guess this doesn't make hashes work, I'm still inputting objects of type MyStruct, and it seems to fail when you give it a hash :/

@connorshea
Copy link
Contributor Author

@connorshea
Copy link
Contributor Author

Interestingly, this errors:

# typed: true
require 'sorbet-runtime'
extend T::Sig

sig { params(options: Hash).void }
def method(options)
  # This isn't allowed
  puts options.x
end

method({x: 2})

While this is fine:

# typed: true
require 'sorbet-runtime'
extend T::Sig

sig { params(options: Hash).void }
def method(options)
  # This is fine
  puts options[:x]
end

method({x: 2})

Regardless, neither lets me set specific keys the Hash needs to have.

@connorshea
Copy link
Contributor Author

Outputting a Shape seems to work somewhat, but Sorbet doesn't allow you to use Hash keys as methods currently.

Input:

# Create a new RBI generator.
# @param [Hash] options
# @option options [Integer] break_params
# @option options [Boolean] replace_errors_with_untyped
# @option options [Boolean] comments
# @return [RbiGenerator]
def initialize(options)
  # code
end

Output:

sig { params(options: { replace_errors_with_untyped: T::Boolean, break_params: Integer, comments: T::Boolean }).returns(RbiGenerator) }
def initialize(options); end

With this, Sorbet handles things a bit better, but options.comments is still a type error, despite being valid Ruby. It only allows options[:comments].

@AaronC81
Copy link
Owner

AaronC81 commented Jun 30, 2019

options.comments is still a type error, despite being valid Ruby

Is that valid Ruby? At least with Ruby's built-in hashes, I don't believe it's possible to access hash keys using methods. For example, this errors:

def foo(options)
  p options.a
end

foo(a: 3)

(Please say if I've misunderstood or I'm missing something!)

Also, unfortunately, shapes would be perfect, except for their annoying quirk that (as far as I know) you can't have optional shape keys. Even if you specify a key's value as T.nilable, the caller needs to actually specify it as nil explicitly for it to pass typechecking. Here's a sorbet.run to demonstrate.

@connorshea
Copy link
Contributor Author

@AaronC81 I just kind of assumed it'd work since it works in Sord (assuming options is a hash, which AFAIK it is):

def initialize(options)
  @rbi_contents = ['# typed: strong']
  @namespace_count = 0
  @method_count = 0
  @break_params = options.break_params
end

Does that code work if you do this?:

def foo(options)
  p options.a
end

foo({a: 3})

@connorshea
Copy link
Contributor Author

oh, I guess it doesn't... Then how does the code in Sord work 🤔 Is it not a Hash?

@AaronC81
Copy link
Owner

Whoops, that is documented incorrectly then! I'll change that.

That method shouldn't be passed a Hash; rather, it's expecting a Commander::Command::Options instance, which is essentially a container for a Hash with a method_missing which looks up its keys. Sorry for any confusion!

@connorshea
Copy link
Contributor Author

That explains a lot! :D

@connorshea
Copy link
Contributor Author

Out of curiosity, should we just pass it as a Hash instead of a Commander::Command::Options object so it's easier to get Sorbet to understand it? e.g. in the executable you'd take the options object and .to_h it before passing it to the initializer.

@AaronC81
Copy link
Owner

That'd be a good plan! I've just opened #65 for this.

@connorshea
Copy link
Contributor Author

As part of this, I'm kind of wondering if we should have a flag to only generate Sorbet types that are 'stable', e.g. Shapes and Tuples are marked as incomplete/unfinished in the docs. I'm not sure if we should have Sord generate those by default if it's potentially unsafe to do so.

Just a thought, I'm not really sure it'd be worth implementing since Sorbet itself is pretty unstable so it's to-be-expected that there would be breaking changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants