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 IO a class #4901

Merged
merged 1 commit into from
Oct 14, 2017
Merged

Make IO a class #4901

merged 1 commit into from
Oct 14, 2017

Conversation

asterite
Copy link
Member

This changes IO to be a class instead of a module. The reasons are:

  1. IO has an @encoding variable that can be set and changed. This makes IO as a struct very inconvenient because its mutable.
  2. There are no struct IOs in the standard library.
  3. Less code is generated (code for module dispatch is huge and repeats in every method call). This point can probably be improved by improving the compiler, but the other two points are still relevant.

Because Zip::File now works with IO instead of a union of two specific IOs (and this can't be represented in the language because the types get merged to the closest ancestor) I added raising methods like seek and pos, which also make sense because all IOs should provide that interface, even if they can't effectively implement it (otherwise, let's break IO into many small interfaces like in Go, that's acceptable too but a much bigger and boring change).

@RX14
Copy link
Contributor

RX14 commented Aug 30, 2017

I was about to PR creating a Crystal::System::FileHandle class which will conflict with this. Some coordination between the two will be required as I changed the semantic of seek to return the absolute offset and improved the docs. Is removing tell a good idea, as an alias of pos?

@RX14
Copy link
Contributor

RX14 commented Aug 30, 2017

Making seek return the new position has the advantage of allowing you to implement pos and pos= entirely in terms of seek, which means inheriting classes only need to implement seek.

@asterite
Copy link
Member Author

Yes, I think we can remove tell

@asterite
Copy link
Member Author

(I'll leave it in this PR, though)

@asterite asterite requested review from RX14 and bcardiff September 14, 2017 21:00
Copy link
Member

@bcardiff bcardiff left a comment

Choose a reason for hiding this comment

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

I can't think of an example where a class IO won't work and a module would be better. Yet I always liked that acting as an IO didn't require a base class.

It's hard to search "include IO" in all crystal repositories in github, from a wider search there are many occurrences that can be found. There are some usages in db drivers.

Would it be to much of a burden to:

  • in vNext

    1. declare IO::Base abstract class
    2. make IO print a deprecation warning
  • in vNext+1

    1. remove module IO
    2. change IO::Base to IO
    3. make IO::Base < IO with a deprecation warning
  • in vNext + 2

    1. remove IO::Base

Probably yes. The error will be point easily. But without that there is a hard dependency requirement for shards and compiler version IMO.

@active_delimiter_buffer : Bytes

# Creates a new `IO::Delimited` which wraps *io*, and can read until the
# byte sequence *read_delimiter* (interpreted as UTF-8) is found. If
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't the delimiter be interpreted in the encoding of the underling io?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe, but we can discuss that in a separate PR :-)

@@ -1,12 +1,10 @@
# The `IO::Buffered` mixin enhances the `IO` module with input/output buffering.
# The `IO::Buffered` mixin enhances an `IO` with input/output buffering.
Copy link
Member

Choose a reason for hiding this comment

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

WhyIO::Buffered should be a module and IO::Delimited a class?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could make IO::Buffered a class too, but then you'd have to do like in Java:

io = File.open("some_file")
io = IO::Buffered.new(io)
io.gets

otherwise things will be very inefficient. That's why in Ruby buffering is embedded in these kind of IOs because it's almost always slow otherwise.

We could have IO::Buffering as a module and IO::Buffered as a class, though, or something like that.

Still, maybe a discussion for another issue :-)

@asterite
Copy link
Member Author

@bcardiff I don't know about those steps. Maybe it's better to just break things, given that the fix is really simple. This will make shards that depend on other shards break and people filling PRs to fix this.

@asterite
Copy link
Member Author

Bump!

@asterite
Copy link
Member Author

Oh, this is actually approved. I'll use the "If a crystal-lang org member creates a PR, at least one other member has to approve it" card here ♣️

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.

3 participants