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

Replace #status_code with #status #7247

Merged
merged 27 commits into from Apr 5, 2019

Conversation

@dwightwatson
Copy link
Contributor

commented Jan 2, 2019

This is a follow-up to #7231 but broken out into another PR because the direction changed a bit from the discussion over there - thought it might be wise in case that direction is reversed. This basically introduces a number of breaking changes:

  • #status_code on the response object is replaced with #status which is an instance of HTTP::Status. It encapsulates everything needed to do with HTTP status codes.
  • HTTP.default_status_message_for was replaced by the HTTP::Status object's description and it will now return nil instead of an empty string when it doesn't have an answer.
  • You can still set status_code= if you like, and it will be mapped under the hood to an instance of HTTP::Status.
    *HTTP::Status has #code so can call status.code to get the integer value instead of needing to know to call status.value.

Open to more guidance and feedback here. Hope it's going in the direction you'd like to see - there were a lot of comments on the last PR and I've tried to take it all into consideration.

@straight-shoota

This comment has been minimized.

Copy link
Member

commented Jan 2, 2019

When starting with a fresh PR, could you please squash the commits into logical pieces (probably similar to your bullet points)? That makes it much easier to review and follow the individual changes. Thanks 👍

dwightwatson added some commits Dec 29, 2018

Break out standalone StatusCode class
Update some relevant specs

Fix method call

Update expectations

Convert class to enum

Revert to hard-coded messages

Actually revert to class method

Fix overload implementation

Rename StatusCode to Status
Rename default_message_for to just message
Flip methdos back to instance methods

Actually flip methods

Update more uses

Rename #message to #description
Return nil instead of empty string
Update file comment

Co-Authored-By: dwightwatson <dwightwatson@users.noreply.github.com>

Replace #status_code with #status

Update OAuth2::Client

@dwightwatson dwightwatson force-pushed the dwightwatson:status branch from f85f6a4 to a9254f1 Jan 2, 2019

Show resolved Hide resolved src/http/client/response.cr Outdated
Show resolved Hide resolved src/oauth2/client.cr Outdated
Show resolved Hide resolved src/http/web_socket/protocol.cr Outdated
Show resolved Hide resolved src/http/server/handlers/log_handler.cr Outdated
Show resolved Hide resolved src/http/client/response.cr Outdated
@straight-shoota

This comment has been minimized.

Copy link
Member

commented Jan 2, 2019

This looks good!

There is however still an issue with creating a new HTTP::Status instance from an arbitrary status code number. .new on enums is implicitly defined and technically not part of the public API. As far as I can tell, it can neither be overriden nor documented.
Thus, we'd probably need to use a method with a different name that can be documented as part of the public API and validate the assigned value.
Probably something like this:

# Create a new `HTTP::Status` instance from *status_code* number.
# The values are not limited to defined enum members, but also allow to use custom status codes. Any number in the range 100..999 is allowed.
def HTTP::Status.from_code(status_code : Int32)
  raise "Invalid status code" unless 100 <= status_code <= 999
  new(status_code)
end

HTTP::Status.from_code should be used everywhere instead of .new.

status_message = pieces[2]? ? pieces[2].chomp : ""

body_type = HTTP::BodyType::OnDemand
body_type = HTTP::BodyType::Mandatory if mandatory_body?(status_code)
body_type = HTTP::BodyType::Mandatory if mandatory_body?(status)

This comment has been minimized.

Copy link
@asterite

asterite Jan 2, 2019

Member

Maybe we could move this method to a body_mandatory? in the HTTP::Status enum (it could be done in a separate PR).

This comment has been minimized.

Copy link
@dwightwatson

dwightwatson Jan 2, 2019

Author Contributor

Happy to tackle that one separately.

Show resolved Hide resolved src/http/server/handler.cr Outdated
Show resolved Hide resolved src/http/server/response.cr Outdated
Show resolved Hide resolved src/http/status.cr Outdated
Show resolved Hide resolved src/http/status.cr Outdated
Show resolved Hide resolved src/http/status.cr Outdated
Show resolved Hide resolved src/http/client/response.cr Outdated
@straight-shoota

This comment has been minimized.

Copy link
Member

commented Jan 4, 2019

In light of #7266 suggest to rename Status.code to Status.new because it was only a workaround in the first place and is no longer needed.
The compiler change is non-breaking and goes into 0.27.1, so this feature can safely be merged to 0.28.0 (where this PR needs to go anyway, because it's breaking the API).

NOTE: Implementing this needs a very recent compiler build from master.

RX14 and others added some commits Jan 4, 2019

Update src/http/server/response.cr
Co-Authored-By: dwightwatson <dwightwatson@users.noreply.github.com>
@dwightwatson

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2019

@straight-shoota Do you mean replace .from_code with .new?

And is this the sort of implementation you'd expect - defining self.new instead of initialize and then calling private_def inside of that?

def self.new(status_code : Int32)
  raise ArgumentError.new("Invalid HTTP status code: #{status_code}") unless 100 <= status_code <= 999
  previous_def(status_code)
end
@dwightwatson

This comment has been minimized.

Copy link
Contributor Author

commented Jan 5, 2019

Seems like the tests fail now (but work fine locally). Do the tests not run with a fresh build of Crystal?

@straight-shoota

This comment has been minimized.

Copy link
Member

commented Jan 5, 2019

Specs must pass with latest compiler release and a fresh build from current master. This is to ensure every compiler release can be build with the previous one.
Thus CI is expected to fail, but that's fine. We'll revisit once 0.27.1 is released.

@RX14 RX14 added this to the 0.28.0 milestone Jan 5, 2019

@bcardiff

This comment has been minimized.

Copy link
Member

commented Feb 2, 2019

@dwightwatson I notice you are already merging from master. Once you make the CI happy we need a 👍 from @asterite / @RX14 before saquash merging this PR. Thanks!

@Sija

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2019

Needs a rebase to make CI (with Crystal 0.27.1) green.

@dwightwatson

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2019

Back to green now, please let me know if there is anything more I can do to get this merged in.

@Sija

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

Btw, is this GTG already?

@bcardiff
Copy link
Member

left a comment

I think we are ready to (squash) merge this PR.
With the current status I don't think there is a breaking-change, am I wrong?

@asterite

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

@bcardiff HTTP::Response#status_code is renamed to HTTP::Response#status. This is a breaking change.

@asterite

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

@bcardiff It also changes it from a number to an enum (once you rename status_code to status you'll need to do some more changes)

@bcardiff

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

@asterite but #status_code is preserved as a getter and the initializer with and Int is available also.

@z64

This comment has been minimized.

Copy link

commented Apr 5, 2019

@bcardiff It currently removes the public HTTP.default_status_message_for(int) method, replaced by HTTP::Status.new(int).description

@asterite

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

@bcardiff You are right, sorry for the confusion

@straight-shoota straight-shoota merged commit b42631e into crystal-lang:master Apr 5, 2019

5 checks passed

ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@straight-shoota

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

Thank you @dwightwatson ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.