FuelClient API expects redundant possibly conflicting information

[osrf-migration]

Original report (archived issue) by Louise Poubel (Bitbucket: chapulina, GitHub: chapulina).

---

Several functions on FuelClient accept both a ServerConfig and a ModelIdentifier.

But considering that ModelIdentifier keeps its own Server config, we may need to handle the situation when these two conflict. What's the motivation for passing ServerConfig?

If there's no special need for passing ServerConfig, I suggest we stop using it in version 1, document that it has no effect, and deprecate it on version 2.

[osrf-migration]

Original comment by Louise Poubel (Bitbucket: chapulina, GitHub: chapulina).

---

Tagging @sloretz and @iche033 who might be able to clarify this

[osrf-migration]

Original comment by Ian Chen (Bitbucket: Ian Chen, GitHub: iche033).

---

That sounds good to me. Maybe ModelIdentifier could return a ServerConfig if needed? @sloretz would know more about the FuelClient class

[osrf-migration]

Original comment by Louise Poubel (Bitbucket: chapulina, GitHub: chapulina).

---

Maybe ModelIdentifier could return a ServerConfig if needed?

Yup, using ModelIdentifier::Server(), right?

[osrf-migration]

Original comment by Louise Poubel (Bitbucket: chapulina, GitHub: chapulina).

---

See pull request #56

[osrf-migration]

Original comment by Shane Loretz (Bitbucket: Shane Loretz, GitHub: sloretz).

---

I think the intent was a model identifier would be sparsely populated when passed to those functions, and only models on the server that matched the populated items would be returned. The server must be specified when querying so it makes sense as it's own parameter. On the output side, when working with a bunch of fetched models each has an identifier that says what server it came from.

Maybe ModelIdentifier has too much responsibility. The job of specifying parameters to query could be separated from the job of describing the parameters of a fetched model.

[osrf-migration]

Original comment by Louise Poubel (Bitbucket: chapulina, GitHub: chapulina).

---

I see, yes, perhaps decoupling query params from response params would make things clearer.

It's worth pointing out that the ModelIdentifier's Server is also used for querying sometimes, for example on LocalCache::MatchingModels it is possible to return all the models from the same server.

I'm fine either keeping the sever parameters or removing them, but if we're keeping them, we should at a minimum document which one is the authoritative in case _server conflicts with the _id.Server().

[osrf-migration]

Original comment by Louise Poubel (Bitbucket: chapulina, GitHub: chapulina).

---

pull request #56 has been merged, I'll work on a PR to default deprecating the functions