Add custom_request, remove duplication

[iGEL]

I believe, it's not useful to support every HTTP verb defined everywhere with a custom method. But it should be possible to use those verbs, thus we should introduce a method to trigger requests with a custom verb. Unfortunately, there is already a public request method, so I had to choose a different name.

As requested, this is a rebased & updated version of #111. I wanted to asked to reopen that one, but my changes didn't show up after force pushing, so I opened a new PR.

[dennissivia]

@iGEL Thanks for the PR. Just to make sure I am getting it right. The primary focus of the PR is to have a nicer syntax for non "standard verbs" like thiscustom_request("link", "/uri"), right?
Just want to make sure that I understand the trade off correctly.

[iGEL]

@scepticulous Actually, I don't remember the exact details of this PR that I opened almost 3 years ago. I just rebased it and adjusted so the work wouldn't be lost. 😊

From what I remember and read from the changes here, it's not possible to send custom http verbs at all with rack-test (except of course using private APIs with send 😱). There are some very rarely used verbs in the standards out there and I think, rack-test should allow to use the in some way.

Also, I removed some duplication at the same time, which is also a (small) improvement.

[perlun]

@iGEL sorry, but I think we'll have to rebase it again. 😇 Can it be done?

@scepticulous - what do you think about the change per se?

[dennissivia]

@perlun @iGEL I agree with the objection of the PR but I am not quiet sure about the method name for a public API . I would rather prefer a lot of verb liked names and something like request or http_request as public API.
My other concern is the test syntax. I would like to avoid public_send as the execution step. Can we improve these two points somehow ?

[iGEL]

@scepticulous As mentioned in the description, request is already present. I'm not sure how useful it is (since it allows to make requests without a verb), but I would rather avoid breaking the API for this. Keeping the API stable and introducing the new behavior into request is a pretty awkward. To keep the "verb uri params env" form, which seems pretty natural to me, I would have to do something like this:

def request(verb_or_uri, uri_or_env = nil, params = {}, env_or_nothing = {}, &block)
  if uri_or_env.is_a(String)
    uri = uri_or_env
    env = env_for(uri, env_or_nothing.merge(:method => verb_or_uri.to_s.upcase, :params => params))
  else
    uri = verb_or_uri
    env = env_for(uri, uri_or_env)
  end

  uri = parse_uri(uri, env)  
  process_request(uri, env, &block)
end

So, I could rename custom_request to http_request or make_request. No really good options left 😞 WDYT?

To your second comment: I don't really get it. I basically just moved the spec to a better place (the shared example is only used here) and improved it a bit. public_send is still meta programming, but at least you can't call private methods like send before. Can you specify more, what I should do here?

[dennissivia]

@iGEL thanks a lot for the i depth explanation. And sorry I did not read the diff correctly with regards to the test. So refining my two complains:

1. I think you are right and we should be able to support custom requests. And I think custom_request is still a better name, than my suggestions and I would definitely not change request this way.
2. Sorry for the confusion. I think your change improves the tests with regards to location and the public_send. So that is fine with me.

@perlun @junaruga I suggest merging the PR after a final rebase, since it is an improvement in two ways. However it also showed that the touched code needs general improvement, however I would not block this PR because of that but would try to find time to take a look at it myself before we release 1.0.
Please ACK so that I know if you agree with me here.

[junaruga]

@scepticulous
Sorry. I have no time to check the code right now.
I agree with you. :)

[iGEL]

@scepticulous @junaruga @perlun Thanks for the review 😄

I rebased the commit now, but I also added c32207e, because rubocop was complaining about very long blocks in the specs, which I don't consider a bad thing. WDYT?

codeclimate is failing, because it uses an outdated version of rubocop. %i[] is now preferred over %i().

[dennissivia]

@iGEL thanks. I agree with you about the block length but not exactly with disabling it. In my opinion it is fine to have a higher value for tests but I would not disable it. Can you find the value that is appropriate / necessary at the moment, so something like 2-5 to the number necessary to make rubocop happy?.
When that is done, we can merge. We will address the codeclimate/robucop inconsistency on our own later.

[perlun]

@scepticulous I think it's fine to disable it for specs. Most test suites (like rspec etc) are based on block-based syntax, which tends to generate quite long blocks.

So I am fine with the way @iGEL disabled it, for specs only which is exactly what c32207e did. 👍 on that part from my end.

[iGEL]

Thank you 😊