-
-
Notifications
You must be signed in to change notification settings - Fork 74
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
support semi-colon delimited names #589
Conversation
There are some confusing aspects of this code, I probably wrote a bunch of this so I'm guilty, but revisiting it after so long I felt it wasn't super intuitive, I made some attempts to improve it. Some things which are still a bit clunky are:
|
@@ -38,14 +35,4 @@ var OSM_NAMING_SCHEMA = { | |||
// 'sorting_name': 'sorting' | |||
}; | |||
|
|||
// this property is considered the 'primary name' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was kinda weird and overly fancy, I removed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Over the years we've learned many things you can do with Javascript, but shouldn't :P
109b759
to
c74ea5c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏻
c74ea5c
to
e6fcc23
Compare
e6fcc23
to
2a0a319
Compare
.map(v => v.trim()) | ||
.filter(v => v.length); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
diff --git a/util/parseSemicolonDelimitedValues.js b/util/parseSemicolonDelimitedValues.js
index 08e21ac..39c123b 100644
--- a/util/parseSemicolonDelimitedValues.js
+++ b/util/parseSemicolonDelimitedValues.js
@@ -5,8 +5,8 @@ const _ = require('lodash');
function parseSemicolonDelimitedValues(value) {
return (_.isString(value) ? value : '')
.split(';')
- .map(Function.prototype.call, String.prototype.trim)
- .filter(Boolean);
+ .map(v => v.trim())
+ .filter(v => v.length);
}
module.exports = parseSemicolonDelimitedValues;
\ No newline at end of file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much cleaner after these changes. I had one idea for a further improvement but it's not mandatory at all.
@@ -38,14 +35,4 @@ var OSM_NAMING_SCHEMA = { | |||
// 'sorting_name': 'sorting' | |||
}; | |||
|
|||
// this property is considered the 'primary name' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Over the years we've learned many things you can do with Javascript, but shouldn't :P
langValues.forEach(( langValue, i ) => { | ||
if ( i === 0 ) { | ||
doc.setName( langCode, langValue ); | ||
} else { | ||
doc.setNameAlias( langCode, langValue ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah nice this is much cleaner.
...parseSemicolonDelimitedValues(_.get(tags, 'nat_name')), | ||
...parseSemicolonDelimitedValues(_.get(tags, 'reg_name')), | ||
...parseSemicolonDelimitedValues(doc.getName('en')) | ||
].filter(Boolean); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More fanciness :P
So just to confirm, this is because you can't use ||
when assigning to const
, yeah?
Might actually be a use case for let
in that case, as it's much more clear what the intention is, plus each of these function calls has to always be evaluated, whereas chained ||
will have an early return as soon as there's a truthy value, right?
replaces #581
This PR adds support for semi-colon delimited names in OSM.
see: https://wiki.openstreetmap.org/wiki/Talk:Semi-colon_value_separator
Compared to #581 I added:
''
orfoo;;bar
)name:en
) as well as those inNAME_SCHEMA
.parseSemicolonDelimitedValues()
function to it's own module as it's used in theaddress_extractor
too._primary
key fromNAME_SCHEMA
which probably seemed like a good idea at the time but is unnecessarily confusing.var
instead ofconst
etc.)closes #581