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

Feature Request: explicit method defs for HTTP verbs #1168

Open
dduugg opened this issue Jul 17, 2020 · 12 comments
Open

Feature Request: explicit method defs for HTTP verbs #1168

dduugg opened this issue Jul 17, 2020 · 12 comments

Comments

@dduugg
Copy link

dduugg commented Jul 17, 2020

It seems the top-level HTTP verb methods (e.g. Faraday.get) are handled via missing_method proxying to the default_connection ivar. In addition to being a tad inefficient (admittedly a minor concern compared to typical network request latency), it causes issues with various tools:

Would you be amenable to replacing this implementation with explicit method definitions?

@iMacTia
Copy link
Member

iMacTia commented Jul 18, 2020

Hi @dduugg, rather than explicitly defining the methods, I'd explore a documentation-friendly solution.
We use meta-programming definition in the connection as well: https://github.com/lostisland/faraday/blob/master/lib/faraday/connection.rb#L195

Does that work nicely with the points you mentioned above?
If so, we could replicate a similar documentation snippet for the Faraday module as well.

@dduugg
Copy link
Author

dduugg commented Jul 18, 2020

Hi @iMacTia, thanks for the reply. I don't think the problem is solvable with documentation. Could I trouble you to elaborate on the resistance to explicit definitions? I think that would better frame the discussion. For instance, a documentation approach is likely just as verbose (counting various @!method & @!scope directives, in addition to the method_missing code) than the seven one-line explicit forwarding defs I'm suggesting on Faraday. But I don't know if that's the rationale here (code is read much more than it's written, so I think optimizing for the code writer is in error anyway).

@iMacTia
Copy link
Member

iMacTia commented Jul 28, 2020

There are mainly 2 points that put me on the fence with this change.

  • I personally don't think using delegates is an advantage only for the code writer. I've been using Ruby for years now and like others I came to appreciate the succinct syntax of this language. When I read some code and I find delegates I immediately know what that means and it can potentially condense tens of lines of code into just a couple of them.
  • HTTP verbs proxies are not the only instance in Faraday where we take advantage of metaprogramming, there are many more instances, which most probably suffer from the same issue you've raised here, but have not been noticed yet. Agreeing to this change will either make an exception or create a precedent, both things I'd like to avoid.

But instead of focusing on my resistance to this change, I'd rather discuss about what sort of advantages we're trying to introduce here. I completely understand the issue you're describing and I actually think we should try to solve it if possible!
So if the main issue here is compatibility with IDEs/tools for code suggestions, autocomplete and documentation, then I'm simply proposing to find an alternative solution like documentation comments (we already use YARD quite extensively).
And you're also right when you say this solution is just as verbose, although I'd argue that a comment will not impact readability as much as a list of methods and it's easily recognisable.

@dduugg
Copy link
Author

dduugg commented Aug 7, 2020

Hi @iMacTia, thanks for your reply. We've switched to a different library with better API discoverability and tooling support, but I'll share my thoughts in case you find them useful.

  • Discoverability doesn't preclude the use of metaprogramming (though in this specific case I think explicit defs are the way to go), but it would require replacing the method_missing approach, as previously described.
  • I'm not sure I understand the delegates argument. Faraday doesn't appear to use explicit delegators in handling HTTP verb methods. (Confusion on this point is probably a code smell that the implementation of HTTP verb methods is overly complicated).
  • We'll have to agree to disagree that delegate and the like are an aid to the code writer. I'm working with folks with a couple of decades of collective ruby experience, and we haven't found any readability benefits to the approach here (even discounting the time lost to trying to explore the API via pry, YARD, sorbet, and solargraph). It may be worthwhile to watch a rubyist attempt to explore this API for the first time, at least on this specific point.

Just trying to provide some honest (and hopefully useful) feedback here. I still appreciate the time and effort on this contribution to OSS.

@gurgeous
Copy link
Contributor

Hey, thanks for all your hard work on Faraday! I know this is an old issue, but I tend to agree that discoverability is a bit of an issue. I am working on some middleware and I added this section to my README. My library is geared toward someone who might not have used Faraday before, so it's incumbent on me to explain things. I'll say (somewhat hesitantly) that it wasn't easy to puzzle things out.

image

@iMacTia
Copy link
Member

iMacTia commented Apr 20, 2021

@gurgeous I agree documentation is definitely a front where we could be doing a better job (although we started tackling this by launching the Faraday Website), but I don't see how the suggestion in this issue is going to solve your specific point.

Maybe I'm missing the point because I'm simply used to work in a different way, but if I don't know how to use a library I don't rely on autocomplete and guessing to know what to do.
Instead I check the library documentation and, if that doesn't help, I dive into the code to see how it works.

Hence why I think an improved documentation (visual or in the code) is the way to solve this.
But maybe I'm missing something here or a particular use-case where a better documentation wouldn't help?
Is my understanding of "discoverability" correct?

@dduugg
Copy link
Author

dduugg commented Apr 20, 2021

Is my understanding of "discoverability" correct?

Not in my case. My expectation was to be able to do ls Faraday in pry, followed by $ Faraday.get, etc. if I wanted to dive into a specific method. I feel I've been able to do this effectively with any other gem I've used, rarely (if ever) needing to navigate to a documentation page, hence opening this ticket. (This is in addition to other issues mentioned above, such as static analysis tools failing to recognize code that uses the library.)

Thanks again for hearing us out.

@gurgeous
Copy link
Contributor

I love the Faraday Website! Maybe I can submit a PR with a few suggestions? Happy to help out. I think that sort of effort always pays off for newcomers. Let me know if you would be interested.

In terms of pry + ls Faraday, I too am a fan of that pattern and use it extensively. Moving away from method_missing isn't a bad idea. Ruby is slowly getting pulled in that direction with the rise of Ruby 3, language servers, RBS, etc. I think this is secondary to docs, though.

@olleolleolle
Copy link
Member

@gurgeous We are interested in documentation Pull Requests, find the docs/ directory with a set of Gemfile (and README) which is used to publish the website to GitHub Pages, which we are using. If its usage is unclear, the website updating process, please do open an Issue or a PR for that, as well.

Thanks for caring about Faraday users!

@iMacTia
Copy link
Member

iMacTia commented Apr 20, 2021

I love the Faraday Website! Maybe I can submit a PR with a few suggestions? Happy to help out. I think that sort of effort always pays off for newcomers. Let me know if you would be interested.

@gurgeous absolutely! We always welcome help on improving the documentation or the website, as things that are obvious for us maintainers may not be as obvious for users. Plus we really struggle with time and that often comes at a cost for the documentation. Actually, documentation would be in a much worse state if it wasn't for @olleolleolle continued efforts 😄!

On the pry point (cc @dduugg), I never used it hence why I was probably failing to understand.
I just gave it a try with your additional details and I noticed that ls Faraday::Connection does show the dynamically generated methods as well as the delegated ones 🙌.
So my original understanding that we can't use these sort of Ruby "magic" was wrong, and really the only limit is on using missing_method.

What I was not very fond of, was the introduction of explicit methods like the following:

def self.get(...)
  ...
end

def self.post(...)
  ...
end

# ... and so on ...

But if we can fix pry's discovery by adding dynamic definitions or delegations to the main Faraday module, then that's something I'd be OK with. Sorry for not getting it the first time!

@gurgeous
Copy link
Contributor

No worries about time constraints. Never feel bad for being an open source maintainer! We appreciate your efforts regardless. I've been there too...

I'll start a discussion re: docs to make sure we are on the same page.

@dduugg
Copy link
Author

dduugg commented Apr 20, 2021

@iMacTia How would a user know to look into Faraday::Connection? I've since switched to an HTTP gem with a discoverable API, but I took a quick look at https://lostisland.github.io/faraday/usage/ and none of the code examples explicitly use it (though the last does so implicitly). The examples mostly invoke static methods on Faraday, but they aren't found in pry. I don't know of any other gem that has taken this approach, and I still take the view that it's an anti-pattern.

Thanks, as always, for hearing me out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants