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

Implemented Decodable/Encodable for Cell and RefCell. Fixes #15395. #15411

Merged
merged 1 commit into from
Jul 7, 2014

Conversation

mitchmindtree
Copy link
Contributor

I ran make check and everything went smoothly. I also tested #[deriving(Decodable, Encodable)] on a struct containing both Cell and RefCell and everything now seems to work fine.

@@ -537,6 +538,33 @@ impl<E, D: Decoder<E>> Decodable<D, E> for path::windows::Path {
}

// ___________________________________________________________________________
// Implement 'Encodable and Decodable' for the Cell and RefCell types.

impl<E, S:Encoder<E>,T:Encodable<S, E> + Copy> Encodable<S, E> for Cell<T> {
Copy link
Member

Choose a reason for hiding this comment

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

Could you format this with more spaces:

impl<E: S: Encoder<E>, T: Encodable<S, E> + Copy> ...

(And similarly for the others below.)

@mitchmindtree
Copy link
Contributor Author

Done, anything else?

@mitchmindtree mitchmindtree changed the title Implemented Decodable/Encodable for Cell and RefCell. Fixes #15395 Implemented Decodable/Encodable for Cell and RefCell. Fixes #15395. Implemented Show for RefCell. Jul 4, 2014
@mitchmindtree
Copy link
Contributor Author

I just remembered RefCell was missing an implementation of Show while testing before, so I've added that also.

@alexcrichton
Copy link
Member

There's a good deal of discussion about Show for RefCell over at #14883. I'm not sure if it should be added as part of this PR.

@mitchmindtree
Copy link
Contributor Author

@alexcrichton Ok, I will remove it for now.

@mitchmindtree mitchmindtree changed the title Implemented Decodable/Encodable for Cell and RefCell. Fixes #15395. Implemented Show for RefCell. Implemented Decodable/Encodable for Cell and RefCell. Fixes #15395. Jul 4, 2014
@huonw
Copy link
Member

huonw commented Jul 4, 2014

Hm, the Encodable for RefCell has the same failure problems... maybe we shouldn't add it here either? :(

@huonw
Copy link
Member

huonw commented Jul 4, 2014

Or, maybe we can add it but with a fixme for #15036; after that was fixed RefCell would use try_borrow, returning a encoder.error("attempting to Encode borrowed RefCell") from encode when try_borrow returns None. Thoughts, @mitchmindtree, @alexcrichton?

(Thinking about it, this same approach could actually be used for Show.)

@mitchmindtree
Copy link
Contributor Author

@huonw I like this idea, I think it makes a lot more sense than just letting people continuously run into the need to impl these traits. I'd like to see a consistent way for std traits to be impld for std types (even if it involves polite errors), otherwise the Rust ecosystem could get messy quickly with different libs implementing them in different/unpredictable ways. Also, the usage and purpose for RefCell is very specific, I think your suggestion would offer more than enough clarity for any client usage. If the client isn't happy, then perhaps RefCell isn't the type they're looking for? Anyway, I think this would be a much rarer case than clients wondering why it isn't implemented at all.

@alexcrichton
Copy link
Member

@huonw, that sounds like a good way forward, yes. I would prefer Show to always succeed because the errors there indicate I/O errors while the errors here would generally be indicative of "I couldn't encode this" errors (like encoding a non-string-key-map as json).

@mitchmindtree, could you add a FIXME that @huonw mentioned above, as well as adding some tests to just exercising this basic functionality?

@mitchmindtree
Copy link
Contributor Author

@alexcrichton Is that test up to scratch? Let me know if you were after something more thorough.

edit: Ahh I just realised I didn't include a test that 'fails'. Is that important?

@alexcrichton
Copy link
Member

Looks good to me! It's ok if you have a test that doesn't fail.

Could you squash the commits together as well?

@mitchmindtree
Copy link
Contributor Author

I think I squashed them, but I also think I might be making a commit mess? (embarrassed face)

@alexcrichton
Copy link
Member

No worries! It looks like you've picked up a few commits here and there. I would recommend using git rebase -i to squash the commits together.

When done, github should show only one commit as part of this PR. If you need some help, just let me know.

@mitchmindtree
Copy link
Contributor Author

Thanks @alexcrichton ! I tried to do a git rebase -i but it kept showing noop in Vim, without any other options? After experimenting with other suggestions from SO (and making further mess), I decided to simply git reset --hard to the first commit of the PR, and then just re-add the necessary changes with a single extra commit. Let me know if that's alright, or if there's anything else you'd like me to do. Sorry about the wait!

@huonw
Copy link
Member

huonw commented Jul 7, 2014

You also need to specify the point you're starting the rebase from, e.g. if this PR is directly off your master branch, then git rebase -i master (or git rebase -i HEAD^^ to just rebase from the grandparent commit).

…#15395

Updated PR with fixme and test

Updated PR with fixme and test
@mitchmindtree
Copy link
Contributor Author

Wipes sweat off forhead OK, I think it worked! Thanks @huonw, I've learned a lot about git in the last 24 hours 😸

bors added a commit that referenced this pull request Jul 7, 2014
I ran `make check` and everything went smoothly. I also tested `#[deriving(Decodable, Encodable)]` on a struct containing both Cell<T> and RefCell<T> and everything now seems to work fine.
@bors bors closed this Jul 7, 2014
@bors bors merged commit 0e84d6f into rust-lang:master Jul 7, 2014
@lilyball
Copy link
Contributor

I didn't see this when it first happened, but I don't think Encodable/Decodable belong on RefCell, for the same reason we rejected Show for RefCell (#14883).

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

Successfully merging this pull request may close these issues.

5 participants