Subclass Options structs using a block

[bdewater]

Description

As recommended on https://ruby-doc.org/core-2.2.2/Struct.html#method-c-new and https://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Style/StructInheritance

Has the nice side-effect of making the methods defined by Struct.new discoverable by Tapioca, which is how I came across the issue in the first place.

Todos

N/A

[olleolleolle]

I'm curious how well YARD can locate the classes like this.

[bdewater]

@olleolleolle better it seems, I ran bundle exec yardoc and found one instance I had missed, which went away after changing it to the recommended form:

[warn]: in YARD::Handlers::Ruby::ClassHandler: Undocumentable superclass (class was added without superclass)
	in file 'lib/faraday/request/instrumentation.rb':8:

	8: class Options < Faraday::Options.new(:name, :instrumenter)

Edit: spoke too soon, the docs itself look weird.

[olleolleolle]

@olleolleolle better it seems, I ran bundle exec yardoc and found one instance I had missed, which went away after changing it to the recommended form:

[warn]: in YARD::Handlers::Ruby::ClassHandler: Undocumentable superclass (class was added without superclass)
	in file 'lib/faraday/request/instrumentation.rb':8:

	8: class Options < Faraday::Options.new(:name, :instrumenter)

Right, so we can log a new issue about YARD overhaul for methods that should be discoverable - it takes a little bit of hand-wrangling to make "synthetic" or manually written doc blocks for code that can not be found by YARD. Help it on its way.

[bdewater]

It looks like YARD can handle const = Struct.new { ... } but not subclasses - makes sense since it works on an AST.

I guess some paths forward could look like:

• use Struct directly and make Options a module that's included
• avoid the usage of Struct altogether and re-implement what's necessary to achieve the same behaviour
• keep PR as-is but implement a custom YARD handler to help YARD render as we want

Edit: your reply crossed mine - I'll open a YARD issue tomorrow.

[iMacTia]

Thank you @bdewater, as a Sorbet/Tapioca user myself I very much welcome this contribution 🎉 !

Unfortunately, a quick test showed that constants defined inside the structs are not behaving like before, so I've requested some changes.

As an additional note, it's worth mentioning that we've had a long standind desire to move away from structs and use proper classes instead, but this will have to be done in a future Faraday v3 release as it will hardly be backwards compatible 😅

I'd also like to introduce type signatures for Sorbet and RBS to ship with the gem ❤️

[iMacTia]

Thank you 🙏 !

[iMacTia]

I'm a bit surprised these Env:: are necessary, but it's not the end of the world 😄

[iMacTia]

I'm following the discussion in the yard repo, it would be good to find a quick-and-easy solution (they mentioned !@parse?) to avoid breaking documentation before merging this in, if possible

[bdewater]

The directive breaks out the classes as before. Methods defined inside the block are not automatically documented anymore, but that was already the case for the accessors defined by Struct due to subclassing.

[iMacTia]

This is already better than the inline doc generate before.
I assume we'll have to add the missing methods manually as we previously did with the accessors?
I'm happy to get this merged as-is and log a separate issue to get this fixed 👍

@olleolleolle you're the YARD expert in the team, are you aware of any quick-fix we can take advantage of and we might be missing 😅?

[iMacTia]

FYI -- #1490

[olleolleolle]

Thanks for merging - that was the right call. @bdewater Thanks for keeping Faraday future-compatible!

[semaperepelitsa]

Alternative way is to reopen the class. That way, you can define constants as usual.

Env = Options.new(:method, :request_body, :url, :request,
                  :request_headers, :ssl, :parallel_manager, :params,
                  :response, :response_headers, :status,
                  :reason_phrase, :response_body)

class Env
  ContentLength = 'Content-Length'
  StatusesWithoutBody = Set.new([204, 304])
  SuccessfulStatuses = (200..299).freeze