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

Added addr:housename to index #458

Merged
merged 4 commits into from
Jun 7, 2020
Merged

Added addr:housename to index #458

merged 4 commits into from
Jun 7, 2020

Conversation

krahulreddy
Copy link
Contributor

For issue #448:

  1. Added housename mapping to mappings.json.
  2. Modified writeName function to include housename in fNames.
  3. Modified filterNames function to set names.get("housename") as default if names is not null and names.get("name") is null.

Reason for including point 3 above:
Current implementation: In filterNames function, if the names map is not null, then names.get("name") is set as the default name. This fails if the map does not contain the name key.

Examples:

Name: [{name:es=La Adelita}]
fNames: [{}]
Name: [{operator=Gasopas}]
fNames: [{}]
Name: [{addr:housename=Caradon}]
fNames: [{housename=Caradon}]

I noticed that many of the places with addr:housename do not have name key. There should either be a hierarchy to select the "default" or the point 3 above could be opted.

@lonvia
Copy link
Collaborator

lonvia commented May 17, 2020

Reason for including point 3 above:
Current implementation: In filterNames function, if the names map is not null, then names.get("name") is set as the default name. This fails if the map does not contain the name key.

This is intentional. filterNames is really only there for handling pure name tags. These are the only ones we want to handle for any of the address bits (city, country, state, street) of a place. The housename is only interesting for the name of the place itself. That's why we have the extra writeNames function there that handles a couple of special names.

@krahulreddy
Copy link
Contributor Author

This is intentional. filterNames is really only there for handling pure name tags. These are the only ones we want to handle for any of the address bits (city, country, state, street) of a place.

If this is the case, I see only one other way of fixing this issue:

When a search is performed, convert() function in ConvertToJson is the one dropping the other names. The getLocalized() function currently returns Null if map != null but the map.get("default") does not exist.

I think we can add a condition to check if map.get("default") is null, then any of the other fields in the map can be returned.

The housename is only interesting for the name of the place itself. That's why we have the extra writeNames function there that handles a couple of special names.

Even though the housename is added in the writeNames, it never really appears in any of the results, since only default is considered.

Let me know if this is the correct approach, or if I am missing something with regard to this.

@@ -77,6 +77,9 @@ private String getLocalised(Map<String, Object> source, String fieldName, String
return map.get(lang);
}

if (!map.containsKey("default")) {
return map.get(map.keySet().toArray()[0]); // Returning first value in the map
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We really should define an order here. Right now you might get a random localized name, if I understand the code right. I suggest: housename, int, loc, reg, alt, old

Also, note that again only NAME has other names than the default and localised ones. So a special handling of the name would be a good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latest commit handles the precedence order you suggested. This handling is only for the name field but in the same getLocalised() function. (If a separate function is used, most of the code will be repeated, so I did not go for it)

if (map.containsKey(key))
return map.get(key);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But now 'default' is the falllback, while it was the first choice before.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If default needs to be the first preference, I see two ways of doing it:

  1. Add default as the first entry in the NAME_PRECEDENCE array.
  2. Change the if condition from (fieldName.equals("name")) to (map.get("default") == null && fieldName.equals("name")).

Which option should I go for?

@krahulreddy
Copy link
Contributor Author

If default needs to be the first preference, I see two ways of doing it:

  1. Add default as the first entry in the NAME_PRECEDENCE array.
  2. Change the if condition from (fieldName.equals("name")) to (map.get("default") == null && fieldName.equals("name")).

Which option should I go for?

Implemented option 1 in the latest commit as you suggested @lonvia.

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.

2 participants