kbgg / qgis-stac-browser

Plugin for QGIS to browse and download assets from STAC APIs
Apache License 2.0
26 stars 9 forks source link

Select extent by current canvas extent or drawing a rectangle #63

Closed suricactus closed 5 years ago

suricactus commented 5 years ago

Fixes #59 . It's a bit confusing with the two snake_ and camelCase conventions in the same project. I'm trying to follow your style, not sure if I manage all the time.

The new extent selector is similar to qgis.processing.gui.ExtentSelectionPanel. Actually I reuse some code from there, however, it's impossible to fully utilize it (depends on processing too much).

Also fixed a bug, that I didn't notice the previous time. As far as I see, the protocol only accepts bbox in geographic coordinates (I didn't find it in the spec, I'll open an issue about this). However, if the layer is in any other CRS, the search fails. I fixed it now.

Another change is that I use PEP484. I would suggest to use it everywhere in the project, it just makes it easier to develop, including for newcomers.

suricactus commented 5 years ago

My bad about the bbox CRS:

    The coordinate reference system of the values is WGS84
    longitude/latitude (http://www.opengis.net/def/crs/OGC/1.3/CRS84) unless
    a different coordinate reference system is specified in the parameter
    `bbox-crs`.

I think keeping everything in EPSG:4326 is easier because passing bbox-crs around is kinda clumsy.

kbgg commented 5 years ago

I agree with the mixed naming convention being confusing. If I recall correctly there was an issue I ran into where I couldn't use snake case in QT Widget names so I had to use camel case for those. I just checked again and I must have been doing something wrong since it seems like snake case works for those. I'll throw up an issue for switching to snake case throughout the project and I'll likely tackle that one.

Something that needs to be done in the immediate future is to create a contributing guide and style guide. It was on my plate to get done but you started putting in PR's before I could get to it!

As for PEP484 I'm not entirely convinced it's appropriate to follow that convention in this project. While I definitely see the benefits of it my biggest priority is to make it as easy to get involved with this project as possible. I'm not convinced that enough people are aware of the syntax and might get scared off when they see the unfamiliar syntax in the code base. I did a quick survey of the top 10 most downloaded QGIS plugins and none of them follow the PEP484 convention.

The rest of the PR looks good though so I think if we remove the PEP484 this should be good to merge.

suricactus commented 5 years ago

Sorry for being so fast with contributions, as I said in the first PR, I was waiting for something similar for years, it makes my workflow much nicer. I will open an issue for discussion about the style guide.

As for PEP484, I see that in the beginning, the syntax might seem a bit funky to people who are not fully in the CS field. However, in the long term, I think it pays off as smart and easy documentation. Even for beginners, who often use IDEs can get type checking for free, therefore making it easier to contribute with fewer bugs. It's a relatively new feature in the Python world (v3.5), QGIS supports Python 3 since Feb 2018 and it's understandable it's not widely adopted yet, especially for plugins that are tens of thousands SLOC. So at least for separate, completely independent files, I would suggest to keep it.

However, if I was not convincing enough, I would remove it, as I see the change as nice to have.

kbgg commented 5 years ago

I'd like to cut a 1.1 release this week and I want this feature to be included. While we wait for the style discussion to pan out I think it's best to keep the code base consistent and not include the type hints. Hopefully it shouldn't be too much work to add in the type hints if we decide to include those.