Skip to content

Conversation

mmarvick
Copy link
Contributor

Fixed a couple of small errata I noticed when going through the tutorial.

@lovelydinosaur
Copy link
Contributor

We've now fixed (in master the representation of ReturnList / ReturnDict to simply return the native dict/list - since they should really just be transparent implementation details for the most part.

I'd also prefer dropping the OrderedDict note - even though it does actually return an OrderedDict I'd still prefer the example to show the more readable dict version.

@lovelydinosaur
Copy link
Contributor

Happy to accept this if updated as noted above, but closing in the meantime.

@mmarvick
Copy link
Contributor Author

Sorry for the slow response to this. Making the changes now. I replaced what I had changed to ReturnDict back to a regular dict.

I'm a little confused as to what you want me to do for the references to OrderedDict. There's one in the section ...then we restore those native datatypes into a fully populated object instance, and there's an array of them in the section immediately after as well. Do you want me to just change both these to show as regular dicts, even though that's not the real behavior?

Thanks!

@mmarvick
Copy link
Contributor Author

mmarvick commented Feb 5, 2015

Hey @tomchristie -- happy to make the changes for you, I'm just a bit confused on what you're hoping for. Thanks for the help!

Copy link
Contributor

Choose a reason for hiding this comment

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

Leave this as a dict - I know it's not exactly correct, but it's more readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tomchristie Gotcha -- makes sense. Thank you! I also added a 3rd element that's a duplicate of the 2nd because there's an extra call to serializer.save() on line 176. Would you prefer I just leave the first two? Another one of those cases where it's not exactly correct with just the 2, but probably more readable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I didn't understand that. Which 3rd element? Which 2nd element?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tomchristie On line 148, you save the snippet with "Hello world". You then serialize it and resave the "Hello world" snippet on line 176. So the item with pk 2 is "Hello world", and there's also an item with pk 3 that's "Hello world" from the second save.

Copy link
Contributor

Choose a reason for hiding this comment

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

Whatever you think it most sensible.

@lovelydinosaur
Copy link
Contributor

@mmarvick - There ya go - those two inline comments.

@mmarvick
Copy link
Contributor Author

mmarvick commented Feb 6, 2015

@tomchristie I've made the edits and committed. I don't see them here -- I assume it's because the PR is marked as closed? I'm still learning my ways around Github, so let me know if there's something I need to do. They're on the master branch of my fork.

@lovelydinosaur lovelydinosaur reopened this Feb 6, 2015
@mmarvick
Copy link
Contributor Author

mmarvick commented Feb 6, 2015

@tomchristie Cool. Should be ready to merge.

@jpadilla
Copy link
Contributor

jpadilla commented Feb 6, 2015

Looks good to me. Thanks @mmarvick!

jpadilla added a commit that referenced this pull request Feb 6, 2015
Small fixes to the tutorial
@jpadilla jpadilla merged commit 235b98e into encode:master Feb 6, 2015
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.

3 participants