Testing qgis_run_algorithm methods #146
Conversation
|
Just fyi, using |
There was a problem hiding this comment.
Congratulations @jannes-m, as far as I can imagine so far, this should work quite well! I simply didn't want to go as far as you did with adding .data in qgis_run_algorithm(). That's a choice to be made carefully though.
The separate review comments are about further improvement (not questioning the change as such), as is the following one:
- for
.data, several other types are to be potentially captured before going to the default method, which essentially ignores.data: at leastlist,qgis_list_input,qgis_dict_input, and the different possibleqgis_output*classes, logical and numeric. These were all covered in the default method ofqgis_pipe().
E.g. following line from qgis_pipe.default() is not yet mimicked:
if (stringr::str_detect(class(.data), "^qgis_output")) {
.data <- unclass(.data)
}About the choice to make such change: it is still questionable to me whether making qgis_run_algorithm() pipe-friendly is worth the trouble if that is at the cost of putting a rather strange argument at the front (seen from an average user's perspective), which makes the function less easy to understand. Also it breaks existing scripts (to avoid that, we would need to choose a new name and still support/deprecate previous one). And it takes more care in supporting all possible .data classes (see above), since you can't use the default method for that (contrary to qgis_pipe()). You'd also need to rework many tests in the package... (And maybe, maybe not, there are still some overlooked consequences, I didn't yet overthink and check all rabbit holes).
I'm really impressed of the possibility created by this nice piece of work! Still I currently incline to being more conservative by providing an as-simple-as-possible function outside the piping context. To me, being able to pipe easily is very welcome and wanted, but splitting that over two more straightforward functions (i.e. with a more limited list of different arguments) is also a way to user-friendliness (similar trade-off as in #140 (comment) BTW, but there the argument ordering feels natural in both cases).
Anyway the experiment is very valuable and you are certainly free to perfection it further. But the question is if it will be the best way to cater for the user (hence to take a decision to actually adopt this).
Further comments by others are most welcome of course. What's your thought @paleolimbot ?
| !!!qgis_algorithm_args, | ||
| .data = NULL, |
There was a problem hiding this comment.
I'm not quite sure, but probably .data = NULL should also be added to the list in line 93 above:
default_run_alg_args <- list(PROJECT_PATH = NULL, ELLIPSOID = NULL, .quiet = TRUE)| qgis_run_algorithm <- function(.data = NULL, | ||
| algorithm, ..., PROJECT_PATH = NULL, ELLIPSOID = NULL, | ||
| .raw_json_input = NULL, .quiet = FALSE) { |
There was a problem hiding this comment.
Here you'll need to add .select = "OUTPUT" and .clean = TRUE as well. Not to make those arguments work, but for the documentation (only the generic will be shown in the documentation, the methods are hidden from documentation and pkg index).
| #' Run QGIS algorithms. | ||
| #' See the [QGIS docs](https://docs.qgis.org/testing/en/docs/user_manual/processing_algs/qgis/index.html) | ||
| #' for a detailed description of the algorithms provided | ||
| #' 'out of the box' on QGIS (versions >= 3.14). | ||
| #' | ||
| #' @param .data some data needed for piping |
There was a problem hiding this comment.
| #' @param .data some data needed for piping | |
| #' @param .data Optional. | |
| #' Passed to the first input of `algorithm`. | |
| #' If `.data` is a `qgis_result` (the result of a previous processing | |
| #' step), `.data[[.select]]` is passed instead. | |
| #' It can also be one of the qgis_output* classes | |
| #' (e.g. `qgis_outputVector`, `qgis_outputRaster`), | |
| #' a character, a numeric, a logical, a list or any other | |
| #' possible algorithm input argument type. |
The 'pipe-friendly' character of the function can be described in general. It is not necessary for this argument to be used in a piping context.
R/qgis-run-algorithm.R
Outdated
| .clean = TRUE, | ||
| .quiet = TRUE | ||
| ) { | ||
| browser() |
There was a problem hiding this comment.
Thanks for reviewing and your comments, @florisvdh. And no worries, I completely agree with you, the .data argument feels somehow wrong. And yes, qgisprocess does not lend itself naturally to piping, so in my opinion, if you have to pipe, use the native pipe and its placeholder to make crystal clear what you intend to do. So probably, the qgis_pipe() is a good compromise, so let's go with that and close this PR.
Thanks for sharing these considerations @jannes-m, and for your enthusiasm and open mindset! |
Hey @florisvdh,
of course, you were right in #31, for method dispatch to work, one needs the same first argument across the methods. Well, this is not entirely true, see example below where method dispatch is always done via the first specified argument, hence, in the first case it is done via the
algorithmargument and in the other examples via the.dataargument, maybe there is a way to further exploit this... For now, I used a.dataargument in allqgis_run_algorithm()methods. It works (see below). However, now that thealgorithmargument is no longer the first, one has to explicitly set it, e.g.,algorithm = "native:buffer".This is just a test PR, so that you can easily compare what I have changed. But feel free to close it without further working on it or merging. I have to admit I am also a bit torn if I like the approach or not. On the one hand, it is great that one does not need another function like
qgis_run_algorithm_p(), on the other hand the approach introduces a.dataargument and still feels a bit rough on the edges.Created on 2023-03-30 with reprex v2.0.2
Session info