Skip to content
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

assignLabels: improve duplicate label handling #1507

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Joxit
Copy link
Member

@Joxit Joxit commented Feb 5, 2021

This is the API part of pelias/labels#42, this will improve the user experience and developer experience.

Sometimes, we can see several identical labels, this is very annoying because we can not distinguish them. This was a long time issue for us @jawg.

Screenshot 2021-02-05 at 13 08 33

Our first response was, this is a integration issue, so update the name when you need to... So we implemented this in our demonstrator to say "hey, it's possible to do it on integration".

image

Now I realize it's awful for the developer experience... I blame myself terribly for not having thought of putting it in the API earlier... Especially since it's not complicated 😭

So with this PR, Bagneux will return:

Bagneux, Hauts-de-Seine, France
Bagneux, Deux-Sèvres, France
Bagneux, Indre, France
Bagneux, Marne, France
Bagneux, Allier, France
Bagneux, Meurthe-et-Moselle, France
Bagneux, Aisne, France

And Sao Paulo will return:

São Paulo, SP, Brazil
São Paulo, AM, Brazil
São Paulo, MT, Brazil
São Paulo, MA, Brazil
São Paulo, BA, Brazil
São Paulo, ES, Brazil
São Paulo, PA, Brazil
Se, São Paulo, Brazil
Frei Paulo, Brazil
Sao Paulo, Brazil

Well known localities are still unchanged, for example Paris:

Paris, France
Paris, TX, USA
Paris, ON, Canada
Paris, TN, USA
Swainsboro, GA, USA
Paris Township, OH, USA
Paris Township, IL, USA
Paris Township, OH, USA
Paris, ME, USA
Paris, IA, USA

TODO: Change package.json before merge

related: pelias/labels#41 pelias/labels#42

@orangejulius
Copy link
Member

Yeah, this is great. We definitely should have built in some simple functionality to "extend" the labels before. And like you said, it's not actually complicated (at least not yet, I bet this system will grow 😇 )

@orangejulius
Copy link
Member

In the spirit of ways to make this more complicated in the future, an additional "extended" label feature could be the street of a POI. That would help with something like this for example :)
image

@Joxit
Copy link
Member Author

Joxit commented Feb 5, 2021

Hum, nice use case ! And I found an issue with my current PR 😅

Police, Paris, France
Police nationale, Paris, Paris, France
Police Nationale, Paris, Paris, France
Police nationale, Paris, Paris, France
Police nationale, Paris, Paris, France
Police Nationale, Paris, Paris, France
Police nationale, Paris, Paris, France
Police Nationale, Paris, Paris, France
Police nationale, Paris, Paris, France
Police Nationale, Paris, Paris, France

@Joxit
Copy link
Member Author

Joxit commented Feb 5, 2021

I drafted something for Starbucks 🤔 (streets only for venues with withOptional = true)

Starbucks, 5th Avenue, New York, NY, USA
Starbucks, 6th Avenue, New York, NY, USA
Starbucks, 3rd Avenue, New York, NY, USA
Starbucks, West 181st Street, New York, NY, USA
Starbucks, 7th Avenue, New York, NY, USA
Starbucks, 1st Avenue, New York, NY, USA
Starbucks, East 93rd Street, New York, NY, USA
Starbucks, East 90th Street, New York, NY, USA
Starbucks, West 145th Street, New York, NY, USA
Starbucks, West 23rd Street, New York, NY, USA

@missinglink
Copy link
Member

This is huge from a user experience perspective, ❤️ it

@Joxit
Copy link
Member Author

Joxit commented Feb 5, 2021

I agree ! 😄

@Joxit
Copy link
Member Author

Joxit commented Jun 8, 2021

PR rebase to master.

The CI failed because of Fastly outage 😕

fetch http://dl-cdn.alpinelinux.org/alpine/v3.8/main/x86_64/APKINDEX.tar.gz
WARNING: Ignoring http://dl-cdn.alpinelinux.org/alpine/v3.8/main/x86_64/APKINDEX.tar.gz: temporary error (try again later)
fetch http://dl-cdn.alpinelinux.org/alpine/v3.8/community/x86_64/APKINDEX.tar.gz
WARNING: Ignoring http://dl-cdn.alpinelinux.org/alpine/v3.8/community/x86_64/APKINDEX.tar.gz: network error (check Internet connection and firewall)
ERROR: unsatisfiable constraints:
  bash (missing):
    required by: world[bash]
  curl (missing):
    required by: world[curl]

Exited with code exit status 2

@Joxit
Copy link
Member Author

Joxit commented Jun 8, 2021

back to normal 👍

@Joxit Joxit force-pushed the joxit/feat/labels-with-optional branch from bcca87e to b4982aa Compare October 8, 2021 09:27
@Joxit
Copy link
Member Author

Joxit commented Oct 8, 2021

Updated and sync with #1565

@missinglink
Copy link
Member

Thanks, I have reviewing/merging this on my TODO list.

@Joxit Joxit force-pushed the joxit/feat/labels-with-optional branch 2 times, most recently from fb586b4 to 1205687 Compare May 19, 2022 19:32
@Joxit Joxit force-pushed the joxit/feat/labels-with-optional branch from 1205687 to fd1d0e5 Compare May 19, 2022 21:02
@Joxit
Copy link
Member Author

Joxit commented May 19, 2022

I've updated this PR to reflect changes from pelias/labels#42

The interesting change here is this part: https://github.com/pelias/api/compare/1205687cf7ec73ac84ac699f4fa88db6f5fb2f31..fd1d0e5273a1439530ab1713a7b189bbbdd3263c

Now getBestLayers returns a Set instead of an array and filterUnambiguousParts returns a function (this will avoid the lint error Functions declared within loops referencing an outer scoped variable may lead to confusing semantics. (filterUnambiguousParts).

@Joxit Joxit marked this pull request as draft January 17, 2023 13:59
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.

None yet

3 participants