Add categories to the places endpoint

[Joxit]

Categories are something really useful (and interesting) that's why they should also be available in the Places API.
This PR will add categories when we use the endpoint places e.g /v1/places?categories&ids=...

[orangejulius]

Looks good to me. Can you add a little section to https://github.com/pelias/documentation/blob/master/place.md ?

[missinglink]

Heya, just checking that this implementation is consistent with how it's done in https://github.com/pelias/api/pull/1273/files#diff-4e1b6a181cc42ca3c8295245f9c4d63e?

I see you added some alwaysBlank code, can you please quickly explain the differences between how it works for the place endpoint and the other endpoints?

[Joxit]

@orangejulius : Okay for the section 👍

@missinglink : When you set the categories query parameter to Search or Autocomplete, you can add a specific category or send an empty one. The empty one will not filter the result by categories and will add a warning message Categories parameter left blank, showing results from all categories.. For the place endpoint, we do not need any category filter, because we know what we really want with its ID. That's why the category should always be empty and should not send warning message either.

[missinglink]

Agh yes I understand now :)
I feel like there is something not quite right about this and I can't quite put my finger on it.

It's probably something that I added actually, where the categories filter can also be used to control the display of categories, two totally different things. I think this has already complicated the use of the ?categories param.

In the case of /v1/search we emit a warning to guide the user if they specify an empty categories param:

Categories parameter left blank, showing results from all categories.

I think in this case, because the /v1/place endpoint can be used to return multiple places I think it should also emit a warning that explains that specifying a non-empty categories value has no effect on the filtering of results?

The message could be something like this:

Categories filtering not supported on this endpoint, showing results from all categories.

example query:

/v1/place?categories=foo&ids=openstreetmap:address:node/6600732186,whosonfirst:borough:421205771

Sorry to be so picky, I just think I've already made it confusing enough I don't want to make it any more confusing for users 😄

[Joxit]

Yes, you are right, a message for non empty categories makes sense 😄
Done with 7a81375

An other solution would have been to always activate the categories for /places 🤔

[missinglink]

Yes I also considered just always showing categories for place.

If we did that then we should really show all fields because otherwise I don't really see the value of /v1/place besides from making migration from Google easier.

One pro for that approach would be that the place API would have no params other than ids which makes is super simple and predicable to use.

I'm actually fine with whichever solution, and I'm totally 100% for adding the ability to view categories in /v1/place.

The question is more about API design and what makes the cleanest and most intuitive API for users?

Julian is out for the Thanksgiving and I'd like to hear his opinion (and everyone elses too!), would you mind if we all had the weekend to think on it some more and merge this next week?

[missinglink]

Also the elephant in the room here is that the categories feature is in this weird position of both not really being supported and also being part of our API for years now albeit hidden by default.

Another option is to finally adopt it as a first-class feature and expose it everywhere.

[Joxit]

I'm ok to have Julian's opinion 😄

Yes, displaying categories in all APIs may also be interesting and maybe more consistent ? Because categories are like addendum and they are always exposed.

[missinglink]

@Joxit sorry I refactored some code today and I seem to have caused a merge conflict 😦 on this branch.

[Joxit]

😿

[Joxit]

Updated 😉

[missinglink]

Let's just go ahead and merge this as-is and we can discuss the idea of making categories "on by default" in another thread.
It doesn't make sense to block this work which a client is waiting on just so we can have that discussion 😄

[Joxit]

Yeah ! 😄 🚀

[missinglink]

Opened #1405 for the meta-discussion on categories.