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

Make HTTP::Handler a module #3708

Merged
merged 1 commit into from Dec 20, 2016

Conversation

andrewhamon
Copy link
Contributor

@andrewhamon andrewhamon commented Dec 15, 2016

Because HTTP::Handler was just an interface, it is less restrictive on applications to be an includeable module rather than in inheritable class.

This is a proof of concept of what I mentioned in #3690

@asterite
Copy link
Member

Before we hit 1.0 we usually don't care about introducing breaking changes. I'd directly make HTTP::Handler a module and make the necessary changes instead of adding another type.

@andrewhamon andrewhamon changed the title Make a Handleable module to replace HTTP::Handler Make HTTP::Handler a module Dec 15, 2016
@andrewhamon
Copy link
Contributor Author

@asterite made the changes!

@@ -371,7 +375,8 @@ module Crystal::Playground
end
end

class EnvironmentHandler < HTTP::Handler
class EnvironmentHandler
include HTTP::Handler
Copy link
Contributor

@RX14 RX14 Dec 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would leave a blank line after here

Because HTTP::Handler was just an interface, it is less restrictive on
applications to be an includeable module rather than in inheritable
class.
@andrewhamon
Copy link
Contributor Author

If someone wants to cancel all those now useless Travis CI jobs that are queued, that would be nice :p I didn't mean to clog the tubes 😅

@asterite
Copy link
Member

Even though this is a breaking change, the fix is easy (include instead of inherit). Let's do this!

@andrewhamon Thank you!

@asterite asterite merged commit dd1dca4 into crystal-lang:master Dec 20, 2016
@asterite asterite added this to the 0.20.2 milestone Dec 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants