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

Add a version using new io module #2

Closed
TyOverby opened this issue Feb 8, 2015 · 25 comments
Closed

Add a version using new io module #2

TyOverby opened this issue Feb 8, 2015 · 25 comments

Comments

@TyOverby
Copy link

TyOverby commented Feb 8, 2015

As I understand, the purpose of this crate is to prepare for a world where we no longer have the endian-writing/reading functions on Reader and Writer. As someone that that uses those functions a lot, I would like to prepare my crates (namely bincode) for the new io crate. I'd be willing to help do the port if you are interested.

@BurntSushi
Copy link
Owner

You should be able to use byteorder today with minimal changes. e.g., replace

self.reader.read_be_i64().map_err(wrap_io)

with

self.reader.read_i64::<BigEndian>().map_err(wrap_io)

You'll need to import ReaderBytesExt and BigEndian from byteorder.

Also, I'm on IRC (nick: burntsushi). Ping me. :-)

@TyOverby
Copy link
Author

TyOverby commented Feb 8, 2015

But byteorder still expects the reader to be old_io::Reader right?

@BurntSushi
Copy link
Owner

Oh! I see what you meant now. Yes. Currently using old_io::Reader. I think the transition to the new io::Read will be very easy because I explicitly did not use any of the extension methods that are going to get removed (I think).

If you want to start the port, that's fine with me. I probably don't want to merge to master until everything is fully baked in std though. Not sure when that's happening? I guess before the beta?

@TyOverby
Copy link
Author

TyOverby commented Feb 8, 2015

Sounds good. How about a separate branch that tracks new io? I'll get started on the transition today or tomorrow.

@BurntSushi
Copy link
Owner

Perfect. :-)

@BurntSushi
Copy link
Owner

It looks like @tanadeau already did this! tanadeau@221dcc8

@tanadeau
Copy link

@BurntSushi Could you create a branch so I can create a PR to it? That's assuming you still want it to be separate for now.

@BurntSushi
Copy link
Owner

@tanadeau I've just pushed a branch called newio. I'm not sure exactly when I should be making the switch, is std::io and std::path ready for prime time?

@tanadeau
Copy link

I just created the PR.

I think the new modules are good for regular use, but there are still missing pieces. I'm going to start using them and this library for a project I'm creating so I'll find out soon myself.

@tanadeau
Copy link

Thanks for merging!

It might be worth updating the README.md on master to mention the newio branch.

@blackbeam
Copy link
Contributor

What do you think if i merge newio branch and provide it as an optional crate feature?
I think it will be non-breaking change.

@tanadeau
Copy link

Sounds good to me

@blackbeam
Copy link
Contributor

Great! @BurntSushi, what do you think? May be a lot of typing, so i want to make sure you will merge it.

@BurntSushi
Copy link
Owner

I really don't want to have both live at the same time in the same branch.

Can you tell Cargo.toml to use a specific branch in the mean time?

I'd like to just move everything over to the new io stuff, but I don't know when the right time to strike is.

@blackbeam
Copy link
Contributor

I agree with you to be honest.. just my impatience.

I think you can, but it's not an option for maintainers of cargo packages because you can't publish a package with git dependencies.

I think when both Read and Write become stable (around Mar 9)

@danburkert
Copy link

👍 for moving over to the new IO APIs. As I understand it the functionality provided by this library isn't really necessary with the old APIs, but it is definitely helpful with the new APIs. I am running into this right now porting a custom Serializer/Deserializer to the new IO APIs. Perhaps cut one final release before moving over?

@TyOverby
Copy link
Author

👎 for moving to the new IO apis before they are stable. Keep old API until we no longer need feature(io).

@tanadeau
Copy link

If you want to use this library with the old IO libraries, why not just pin the version in your Cargo.toml to the last release?

I'm not sure what's gained by keeping byteorder against a deprecated API that, as @danburkert mentioned, needs it less.

@BurntSushi
Copy link
Owner

I don't think we need to wait for official stability. There is still plenty of unstable stuff to go around, and as others have mentioned, the utility of this library is tied to the new io library.

I will desperately try to find time this weekend to get this released with the new io. (The work is done for this library, but I'll have to fix my downstream dependencies.)

@BurntSushi
Copy link
Owner

@sfackler I think you're using this crate too? Do you have any strong opinions?

@sfackler
Copy link
Contributor

I'm going to be switching rust-postgres over pretty soon.

It also seems like it should be pretty painless to support both old io and new io at the same time.

@TyOverby
Copy link
Author

@sfackler I think the argument is mainly "which version should be the one that is the current version on cargo".

@sfackler
Copy link
Contributor

@TyOverby byteorder can just define both ReaderBytesExt and ReadBytesExt and the same for Writer. It's not like they're totally incompatible or anything.

@TyOverby
Copy link
Author

I really don't want to have both live at the same time in the same branch.

@BurntSushi
Copy link
Owner

So I just bit the bullet and added support for std::io in d3f22dc. I'm still supporting std::old_io as per @sfackler's suggestion. Unless there is some other complication, I'll keep it this way until std::old_io disappears.

I've updated the docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants