[api] Remove #parse_response spec helper
#7143
Conversation
| @status = API_STATUS[@code] || (@success ? Rack::Utils.status_code(:ok) : Rack::Utils.status_code(:bad_request)) | ||
| @message = @result.kind_of?(Hash) ? @result.fetch_path("error", "message").to_s : @result | ||
| @success | ||
| def result |
There was a problem hiding this comment.
Naming is hard... Looking at the examples in this PR, I wouldn't know what result meant at just a glance. response_body, response_hash or something?
There was a problem hiding this comment.
Yes, I agree...I kept at result to minimize the change from @result, but agree a more descriptive name would be better. Maybe parsed_body? @abellotti any thoughts?
There was a problem hiding this comment.
Yeah, parsed_body is good but the naive part of me wants to see response in the name.
There was a problem hiding this comment.
I like response_hash...similar to response_body, but clearly converted to a Hash, however it might not match the unauthorized case.
|
I like this change, just not sure of the new method name. 👍 |
| @@ -85,7 +85,7 @@ | |||
|
|
|||
| it "can fetch a specific result as a primary collection" do | |||
| report = FactoryGirl.create(:miq_report_with_results) | |||
| result = report.miq_report_results.first | |||
There was a problem hiding this comment.
Wow, this would have been confusing with the prior iteration. 👍
I often use result as a variable(naming is hard) so using response_hash makes instances like this less confusing.
|
👍 👍 |
|
<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush. |
|
Looks great! 👍 |
Each request method was calling `#parse_response` after the request was submitted. This method was doing some things that were either unused, or could be easily replaced by a method built into the request spec framework. In particular: * References to `@code` could be replaced with `response.status`, or use the built-in http status matcher * `@success` was not used externally * `@status` was not used * `@message` was not used * `@result` was used quite extensively, so was replaced with a method The net result is that the state has been reduced, as has the work done on each request.
Changing this method name to the more descriptive `response_hash`. The original idea was to make this method only responsible for parsing JSON from the response body, and to update any callers of it to do something else where it was switching before. As it turned out, all callers could happily parse the response body, which simplified the whole thing.
imtayadeway
force-pushed
the
api/delete-some-spec-code
branch
from
67e4973
to
38a6f5b
Compare
|
Checked commits imtayadeway/manageiq@65da02d~...38a6f5b with ruby 2.2.3, rubocop 0.37.2, and haml-lint 0.16.1 |
|
@jrafanie @Fryguy @gtanzillo Just rebased to make this mergeable again |
|
Looks great, thanks @imtayadeway 👍 |
[api] Remove `#parse_response` spec helper
|
🎉 So much better |
Each request method was calling
#parse_responseafter the request wassubmitted. This method was doing some things that were either unused, or
could be easily replaced by a method built into the request spec
framework.
In particular:
@codecould be replaced withresponse.status, or usethe built-in http status matcher
@successwas not used externally@statuswas not used@messagewas not used@resultwas used quite extensively, so was replaced with a methodThe net result is that the state has been reduced, as has the work done
on each request.
/cc @jrafanie ✂️ 🔥 etc..
/cc @abellotti @gtanzillo
@miq-bot add-label test, refactoring, api