geometalab / Vector-Tiles-Reader-QGIS-Plugin

Vector Tiles Reader QGIS-Plugin - QGIS Python plugin which reads Mapbox Vector Tiles from a server, a local MBTiles file or a directory
http://plugins.qgis.org/plugins/vector_tiles_reader/
GNU General Public License v2.0
150 stars 30 forks source link

Formalize and stabilize public API #237

Open tomchadwin opened 5 years ago

tomchadwin commented 5 years ago

I call VTR methods from another plugin. I currently do so as follows:

from vector_tiles_reader.util.tile_json import TileJSON

and:

vts = layer.customProperty("VectorTilesReader/vector_tile_url")

Because this is not a formal public API, it is possible that these could change, and my code would stop functioning.

There are various things which could help avoid this:

  1. Use the Python convention of prefixing private methods/properties/functions with _. This won't prevent unintended changes to a public API, but it's a helpful reminder:
  1. Abstract the public API from the private calls above, allowing the private ones to be renamed or changed as required in future VTR dev.

  2. Adopt semver for releases (might be in place already) to make it clear to developers when the public API breaks

  3. Once implemented, register the public API in the new qgis.plugins object:

It's perfectly possible that this is of limited use, and that no-one is calling VTR from elsewhere. It doesn't seem a bad idea to me, though, and can probably be done piecemeal.

Interested to hear what others think.

mnboos commented 5 years ago

So which API would I register? The TileJSON, the AbstractSource, the VtReader? How about the decoupled APIs using Qt signals? I'm afraid it wouldn't be as easy as registering one API.

I wasn't aware, you were using this

from vector_tiles_reader.util.tile_json import TileJSON

Can you show me some examples of your code, so that I get a better idea of your kind of usage?

Regarding your ideas:

Use the Python convention of prefixing private methods/properties/functions with _. This won't prevent unintended changes to a public API, but it's a helpful reminder:

I'm already doing that. However, that's probably not of too much use for you, since even the public APIs might change.

Abstract the public API from the private calls above, allowing the private ones to be renamed or changed as required in future VTR dev.

By this you mean like creating an interface class that is implemented by the classes?

Adopt semver for releases (might be in place already) to make it clear to developers when the public API breaks

You're right, I'm not consistently doing that.

I see your intention and I'm aware of your usage of the layers custom property, which I won't change due to this. However, doing so would really put some chains on the development which, obviously, I don't like so much. Especially, since this is a plugin and not some kind of SDK or public service, then it would of course be a totally different story.

cc @sfkeller

tomchadwin commented 5 years ago

Thanks for the detailed reply, and I totally accept all your points. As for your last point - you're not a public service - and your first question - what to expose - I guess the high-level answer is this:

VT layers are an entirely new layer type. Plugins interact with layers all the time. Other plugins might well therefore need to know the core information about a VT layer.

What the API should expose is:

So I guess my initial suggestion was merely to make the API that I already informally use more reliable and protected from change. Yes, an interface class could do that, but alternatively making use of the new plugins object recently committed to QGIS could reduce the work.

I wasn't aware, you were using this from vector_tiles_reader.util.tile_json import TileJSON Can you show me some examples of your code, so that I get a better idea of your kind of usage?

To be honest, I can't remember why I needed your TileJSON(), so I'll look into it and let you know.

tomchadwin commented 5 years ago

Here's my use of TileJSON, though I can't remember precisely why I'm doing all of this (it was a while ago):

def VTLayer(json_url):
    sln = safeName(json_url)
    try:
        key = json_url.split("?")[1]
    except:
        key = ""
    json = TileJSON(json_url)
    json.load()
    tile_url = json.tiles()[0].split("?")[0]
    key_url = "%s?%s" % (tile_url, key)
    vtJS = """
        var layer_%s = L.vectorGrid.protobuf("%s", {
            rendererFactory: L.svg.tile,
            vectorTileLayerStyles: style_%s
        });""" % (sln, key_url, sln, sln)
    return vtJS