extend client with a method to read all documents

[jgillich]

Work in progress, still lacks a representation of the documents in ViewResponse, deserialization of the rev etc. Hoping to get some feedback, because I don't really like this in its current form.

Adds a DatabaseViewPath, bascially the same as ViewPath but without a design document. ExecuteView then stores them in an enum, which makes accessing the path really awkward. Maybe ViewPath itself should be an enum, or can you think of something better?

ref #39

[cmbrandenburg]

The DatabaseViewPath (and DatabaseViewPathRef) types represent a URI path of the form /db/_all_docs. If I understand this correctly, then view_name should always be the constant _all_docs, and hence view_name is redundant because it doesn't store any information. I suggest removing the view_name field and modifying iteration to always return _all_docs for the view name path segment.

[jgillich]

There are other paths below db, like /db/_temp_view, that could also be represented by this. But the names could definitely be constants.

[cmbrandenburg]

Good point. Long-term, we'll need something that handles additional database sub-paths—be it one type with a sub-path &str or a macro that defines a unique Rust type for each sub-path.

[cmbrandenburg]

Overall, there's a lot of really good work here. Here's some feedback.

If /db/_all_docs is to overload ExecuteView as its action type, then you'll need to add some type parameterization, otherwise the application can say something like:

cient.read_all_documents("/baseball/_all_docs")
    .unwrap()
    .reduce(true) // !! Has no meaning for `_all_docs` !!
    .run();

The ideal is for the application to be incapable of expressing nonsensical actions. For _all_docs, this means also excluding the view-execution query parameters group, grouplevel, and maybe attachments (does CouchDB really not support this or are my docs incomplete?).

However, adding another type parameter to ExecuteView may cause headaches. It my be better to have two distinct action types that share code under the hood—something to keep in mind.

As for the enum path stuff, I can't think of anything better, but at least it's not exposed in Chill's API. I'll continue thinking about it over the next few days and maybe come up with a fresh idea.

Client::read_all_documents looks good insofar as taking a database path—that's perfect. You can take this further and nix DatabaseViewPath and its non-owning cousin, DatabaseViewPathRef. These types maybe could be replaced by an adapter type that wraps a DatabasePathRef and implements an Iterator that tacks on an extra _all_docs path segment after the database name. I envision the application programmer never typing _all_docs anywhere in their source, which is what you're already setting up for—awesome!

Just as a heads up, I'll be out of town for a few days starting tomorrow (Saturday), and I'm not sure whether I'll have Internet access, so I may be a bit unresponsive during that time.

[jgillich]

Like 70 LOC just to deserialize a single value object..maybe you know a shorter way to do this.

[cmbrandenburg]

The alternative is to use serde_codegen, which introduces its own complexity. For Chill, I prefer to hand-roll serialization and deserialization because:

1. The CouchDB API is stable, so,
2. Once you write the code for implementing Serialize or Deserialize, you're done, and the code will rarely change, if ever, and,
3. The code is mostly copy-and-paste anyway so it's not like it's hard to do.

Whereas, if we needed to continually change our JSON representations because, say, the HTTP API were continually changing, then the advantage would shift in favor of using serde_codegen and we would eat the extra build-time complexity.

Someday, when compiler plug-ins are stabilized in Rust, it will be a no-brainer to rip out the hand-rolled code and use serde macros.

[jgillich]

No doubt, writing them by hand avoids a whole lot of problems. I just find it hard to believe that you'd need that many lines for a seemingly simple task. I recently implemented a deserializer in Elm and that took a single line per field, with static typing and all (although it compiles to JS, maybe that's why). I guess I'm just spoiled by dynamically typed languages. :)

[jgillich]

ViewRow implements PartialOrd, but uuid::Uuid doesn't (part of document). I'm not too familiar with these traits, any idea how to store a Document in ViewRow?

[cmbrandenburg]

Look like this will help.

[jgillich]

Even with that change: https://travis-ci.org/chill-rs/chill/jobs/124559345#L335 😱

I think I'm just going to add some more tests for all_docs and finish include_docs later. Although I'm not sure how to fix this, other than through removing some traits or implementing them ourselves (ugh).

[cmbrandenburg]

Did you update our Cargo.toml to use v0.2 of the uuid crate? Looks like we're still on v0.1.

[jgillich]

Oops how did that happen, I could swear I ran cargo update and it said 0.2.1...

[cmbrandenburg]

One of the dependencies pulls in uuid v0.2. If you grep the build log for uuid then you'll see both v0.1 and v0.2 getting pulled in.

[jgillich]

Sorry for the delay, I've dropped include_docs and rebased on master, should be good to go now.

[cmbrandenburg]

Awesome! Just give me a few days to review.

[cmbrandenburg]

I would like to rename read all documents to something more like execute all docs view. As you've pointed out, the action is more like executing a view than reading a document, so the name should connote that. Maybe ExecuteAllDocsView? Do you have a better idea?

Maybe we can get this pull request in by the v0.1.2 release, which I'll be making this weekend. We also need some integration tests for your new action, but I can write those if needed.

In other news, I just pushed up a new branch, which will start the v0.2.0 development. The main change is that it vastly simplifies the path-related types, at some cost to run-time performance. The master branch will track v0.1.x until we're ready to commit to full-time v0.2.x development.

[jgillich]

ExecuteAllDocsView is quite verbose. I would actually prefer shorter names like all_docs, put etc (pouchdb), so a action call could fit into a single line. But it doesn't matter that much..

[jgillich]

Also, 👍 for the path changes. The performance difference is absolutely irrelevant.

[cmbrandenburg]

I agree about the verbosity. How about GetAllDocs?

I like the verb + noun convention. Also, I had been avoiding verbs such as get and post so as to not have them conflated with HTTP methods, but this probably isn't important. In the future GetAllDocs will support HTTP POST when retrieving an explicit list of document ids, but having get sometimes mean POST isn't such a big deal.

We can rename ExecuteView to GetView in v0.2 for consistency.

[cmbrandenburg]

I released v0.1.2. You should merge/rebase on master again, which now has the upcoming v0.2.0 changes. There will be conflicts, but the upside is your pull request will shrink—e.g., DatabaseViewPathRef will go away.