qgis_extract_output_* functions

[JanCaha]

Continuation of #139 :)

@florisvdh Take a look and let me know if this goes in the direction that you had in mind. I am not quite sure that I got your notes from #133 correctly. So feel free to comment.

[florisvdh]

Thanks a lot for your work @JanCaha; I'll come back to it soon!

[florisvdh]

OK thanks, here are some remarks to help you further:

• the compat-* tests yield errors because you need to also replace qgis_result_single() in the code of st_as_sf.qgis_result(), qgis_as_raster.qgis_result(), qgis_as_brick.qgis_result() etc. by qgis_extract_output_by_class()
• qgis_extract_output_by_class(), to be able to return multiple elements just like the sibling functions, should gain an argument like single – I suggest default single = TRUE in order to trigger selecting the OUTPUT (if available) or (else) the first element that matches class (currently that is always the case, but can't be disabled). Also, I suggest to replace the argument name what by class (also in the roxygen docs).
• qgis_extract_output_by_name(): great; I'd just replace the which argument by the name argument (except when integrating, see further). The default name = "OUTPUT" is a nice addition to match previous function. In this light you could extend this further with argument single = TRUE (default) in order to always return the first element in case the default OUTPUT is missing.
• qgis_extract_output_by_position(): OK indeed.
• qgis_result_single(): I suggest to drop, it has no added value relative to the others, especially if also adding single to qgis_extract_output_by_name()

You may want to think about an integration like qgis_extract_output(x, which = "OUTPUT", class = NULL, single = TRUE), with which also accepting numeric and with which being ignored if class is not missing. I have a slight preference for that case since it is just one function that the user needs, although it takes a little bit documentation about precedence of arguments, and the effect of single in different situations (which vs. class being non-missing, and probably ignored if which is numeric). In such case, the *.qgis_result() functions would use qgis_extract_output(class = "xxx") and always return a single output. Similarly, just doing qgis_extract_output() will then be sufficient to replicate the older qgis_result_single() with missing class.

[florisvdh]

@JanCaha, you'll also need to fix test-qgis-run-algorithm.R, where qgis_output() is called.

Thanks for adding the tests!

[florisvdh]

These 3 lines are better dropped in each of these new tests, since qgis_result_clean() was already tested in the first test.

[florisvdh]

Additionally @JanCaha – I forgot to think of it in my previous post above – some updates were made to qgis_output() several weeks ago, with related changes in R/qgis-result.R and tests/testthat/test-qgis-result.R (PR #134 is now merged in main; see commits 757b574 and a7e8796). So you'll need to have a look and probably incorporate this too.

Further, you'll also need to fix the conflicts with main that have appeared in this branch. Let me know if you need help.

[JanCaha]

So I did the updates, that you suggested. Hopefully I got your ideas correctly ;-)

The only thing that I did not not include is the complex variant qgis_extract_output(x, which = "OUTPUT", class = NULL, single = TRUE). I kinda feel like it is way to complicated and ambiguous for usage...

Not sure if it is in sync with main right now...need to look at that later.

[JanCaha]

Ok, so this version should be update to date with master and all fixed.

[florisvdh]

Hi @JanCaha thanks a lot for your work; apologies for the delay.

Indeed, the effect of arguments in the integrated approach can be tricky to understand, document and hence use. I agree this is a downside and I'm glad you further refined the alternative.

I invited you as a collaborator to the GitHub repo so that you can push feature branches directly.

• after accepting the invite, please edit current PR at the top, flipping JanCaha:single_output to r-spatial:single_output (they are at the same commit). If you cannot, then best close this PR and continue as a new one, this time using the single_output branch.
• the reason is that you can then review and merge my upcoming PR into single_output, which will add these updates to your PR.

[JanCaha]

@florisvdh thanks.

I am not able to change the source branch for the PR, so I am closing this one and creating a new one. I am also merging #150 to it.