Added the wiki_symbol shield factory #8
Conversation
There was a problem hiding this comment.
So the tricky part here is that it introduces a symbol that might fail in the render() function due to missing data. The interface wasn't designed to expect that.
One option to solve that problem would be to add an additional caching layer. Download and check the image already in create_for and cache the resulting svg in a path defined in wmt_shields/wmt_config.py. Then have render.py simply load and render the local file. You could even work around temporary connection errors that way. You'd just have to come up with a clever system to distinguish between transient and permanent errors.
Also, please add a couple of examples to test/render_test.py and write a test to test_test_shields.py.
| if not (wiki_symbol): | ||
| return None | ||
|
|
||
| print(f'wiki:symbol = {wiki_symbol}', file=sys.stderr) |
There was a problem hiding this comment.
Please use the log library here at debug level.
| print(f'URLs are not accepted in tag wiki:symbol: {wiki_symbol}', file=sys.stderr) | ||
| return None | ||
|
|
||
| match_svg = re.search('(.*)\.svg$', wiki_symbol) |
There was a problem hiding this comment.
Shouldn't that rather be re.fullmatch instead of re.search (i.e. mach the entire string?).
| @@ -60,7 +62,11 @@ def dimensions(self): | |||
| def find_resource(self, subdir, filename): | |||
| subdir_str = str(subdir) if subdir is not None else '' | |||
| filename = str(filename) | |||
| if os.path.isabs(filename): | |||
| if(urlparse(filename).netloc): | |||
There was a problem hiding this comment.
Style: always leave a space after if.
| @@ -60,7 +62,11 @@ def dimensions(self): | |||
| def find_resource(self, subdir, filename): | |||
There was a problem hiding this comment.
I would prefer a separate function find_url_resource here to make it explicit that an external source is needed.
| self.config = config | ||
|
|
||
| def render(self, ctx, w, h): | ||
| data = self.find_resource('', self.url) |
There was a problem hiding this comment.
You need to protect against download failures here or imports will abort at most inconvenient times just because wiki.osm.org is temporarily down.
|
|
||
| def render(self, ctx, w, h): | ||
| data = self.find_resource('', self.url) | ||
| rhdl = Rsvg.Handle.new_from_data(data) |
There was a problem hiding this comment.
And this line may fail because of bogus or broken external data.
| filename = urlquote(wiki_symbol.encode('utf-8')) | ||
| url = f'https://wiki.openstreetmap.org/w/images/{c1}/{c1}{c2}/{filename}' | ||
| print(f' {url}', file=sys.stderr) | ||
| return WikiSymbol(f'wiki_{filebasename}', url, config) |
There was a problem hiding this comment.
Might be saver to use filename here instead of filenamebase to avoid illegal file names.
|
Just to be clear: even though I have objections against using the wiki symbols on waymarkedtrails.org, I have no objections against adding this shield factory. |
This pull request works with an upcoming one on waymarkedtrails-backend. It adds support for SVG files defined in tag wiki:symbol when rendering route shields.
There is on ongoing debate on community.openstreetmap.org about how to manage copyrighted SVG files, but I don't see that it should impact rendering at this stage.