govitia / navitia

A client library in Go for the Navitia API (navitia.io) (v2 WIP)
The Unlicense
6 stars 2 forks source link

Add easy chaining of requests #4

Open aabizri opened 7 years ago

aabizri commented 7 years ago

Reasoning

Most resource endpoints (e.g ../stop_areas/{stop_area}) have sub-endpoints (e.g ../routes) following HATEOS principle. Currently you have to take the resource's ID and call a different method.

Possible solutions

We could simplify this these ways:

For results type having multiple results in them:

- Have a `func (r {Results}) Related() map[ID]map[string](func(context.Context,*Session) Results)` method on the results type, we'll have to create an exported interface `Results` for this to work intelligently.
- Have a single function `func Related(r {Results}) map[ID]map[string](func(context.Context, *Session) Results)` much like the method.

For results type having a single result in them:

- Have each `Results` type that returns only one result implement have an `Explore(ctx context.Context, s *Session, selector string)` method, allowing us easier chaining. (i.e `session.Coords(ctx, coords).Explore(ctx,session,"stop_areas")`).

There may be other ways, I'm open to suggestion !

adelcasse commented 7 years ago

What about something like that ? https://github.com/Ridygo/navitia/commit/b50b1975f4b8c7dce7c83ca5f5d600782ecee8dc

Could be used like this :

scope := session.Scope(types.ID("default"))

params := navitia.PlacesNearbyRequest{
    Depth: 2,
    Distance: 500,
    Types: []string{"stop_area"},
}
coords := types.Coordinates{Latitude: dep.Coordinates[1].(float64), Longitude: dep.Coordinates[0].(float64)}

ctx := context.Background()

// Get stop areas near the departure point
res, err := scope.PlacesNearby(ctx, coords, params)
// fmt.Println(res)
if err != nil {
    fmt.Println(err)
    fmt.Println(res)
}

for _, place := range scope.RelatedContainers(res.Places) {

    opts := navitia.ExploreRequest{
        // ODTLevel: "scheduled",
    }

    // get lines in this stop area
    // lines, err2 := scope.ExploreResource(ctx, place.ID, "lines", opts)

    lines2, err3 := place.Explore(ctx, "lines", opts)
    fmt.Println("LINES :")
    fmt.Println(lines2)
    fmt.Println(err3)
}

(it's a little bit messy right now and needs to be implemented on more types than just Containers)

aabizri commented 7 years ago

I like this general direction, though I have a few questions:

On Results

So Results is a type holding common info for a future request, is that right ? In that case it might be better to name it Proposal or something like this.

Superficial note: a Scope already contains a *Session, no need to store both, and in case where both could be used, it is best to use Scope.

A question: is the separation between SingleValueResults and MultiValuesResults necessary ?

On RelatedContainer

Regarding the RelatedContainers: if I understand correctly this returns a slice of Results which then allows you to call Explore on each result. I like this concept very much, though as you say it needs to be cleaned up still.

On MultiValueResults.Explore

That, I don't understand what you're intending to do. My first impression is, as some XXXResults types can lead to different requests (for example Regions), then we need to be able to explore further per dataset for example, or per disruption. Is that what you're trying to solve ?

The way forward ?

First off, thanks for the input, that's certainly a better way to go about it than the proposed solution in the original post, which either requires the library to have a To{Type} method on results to work easily, or require the user to type-assert, which is bad.

Until your response, I'll see if I can think up something in the same vision as your proposal.

Thanks !