Skip to content

Working get_todos #8

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

Merged
merged 3 commits into from
Nov 19, 2014
Merged

Conversation

iterion
Copy link
Contributor

@iterion iterion commented Nov 16, 2014

It seems like a few rust changes broke get_todos. I had to point to my own fork of nickel.rs while I'm waiting for nickel-org/rust-http#2 to be merged. I'm happy to wait until that is merged and pointing back to mainline nickel.rs before merging this.

I implemented ToJson on Todo instead of on Vec<Todo> primarily because we get Vec<Todo>.to_json() for free when doing so. Also, it seems like it might be better to build a layer on top of the core json serialization stuff to get our output a little more JSON API-ish. I'm not sure if this is the direction we should go, so input here is greatly appreciated.

Implementing ToJson on Vec<Todo> was returning a json body with an
array of strings for the content.
@steveklabnik
Copy link
Owner

I had to point to my own fork of nickel.rs while I'm waiting for nickel-org/rust-http#2 to be merged.

You can use overrides so that you don't end up modifying checked in files: http://doc.crates.io/guide.html#overriding-dependencies

But yeah, let's wait until that PR lands to land this. :)

@iterion
Copy link
Contributor Author

iterion commented Nov 18, 2014

You can use overrides so that you don't end up modifying checked in files: http://doc.crates.io/guide.html#overriding-dependencies

That's awesome, thanks!

Once nickel-org/rust-http#2 is merged I'll update this PR and let you know.

@steveklabnik
Copy link
Owner

👍

@iterion
Copy link
Contributor Author

iterion commented Nov 18, 2014

So, I ended up going down a rabbit hole to get this working, a rust update and few PR's later this is still not quite ready to merge. :)

See:
nickel-org/nickel.rs#118 & sfackler/r2d2#8

Fun!

@iterion
Copy link
Contributor Author

iterion commented Nov 19, 2014

I think everything is good now. A few more things have changed due to changes in rust + dependencies.

steveklabnik added a commit that referenced this pull request Nov 19, 2014
@steveklabnik steveklabnik merged commit f436c55 into steveklabnik:master Nov 19, 2014
@steveklabnik
Copy link
Owner

Woot! Thanks so much!

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.

2 participants