Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revising the namespace #133

Closed
florisvdh opened this issue Mar 7, 2023 · 3 comments · Fixed by #153
Closed

Revising the namespace #133

florisvdh opened this issue Mar 7, 2023 · 3 comments · Fixed by #153

Comments

@florisvdh
Copy link
Member

florisvdh commented Mar 7, 2023

Lately I've started to think about the user interface, i.e. the landscape of functions exposed to the user. Below are some proposed name changes and changes in the choice whether or not to export a function.

Also included is the idea to group functions into function families. Function families can be used for the function documentation (affecting the 'See also' paragraph) and for grouping in the function-reference webpage.

More ideas are welcome.

Ideas to rename and (not) expose functions

These were guided by a few principles:

  • The provision of exported functions that cover the user's needs in a sufficient and adequate manner. The function families help in getting grip on this. Functions that are primarily helpers to these functions, become internal.
  • Function names should ideally be self-explanatory and use verbs – the tidyverse styleguide also refers to the criteria of object names: 'concise and meaningful'.
    • When applied, 'verbs & explanatory' often makes the current names a bit longer – also they already use a 5 character prefix (qgis_). For the most often used functions, we could think of some shorthand synonym, see further for a few ideas.
  • Consistency in the naming approach of related functions, in order to pinpoint their relationship.

When renaming functions, the old names can still remain functional but be deprecated for some time. (Actually, this is not strictly needed for a package in 'experimental' state. See https://r-pkgs.org/lifecycle.html#lifecycle-stages-and-badges.)

In the table below, the 'future' columns are only filled where a change is proposed. It lists 40 functions (i.e. with a proposed change) out of 86 internal+exported package functions (not counting methods).

It is best to also look at the exported functions per family (further) in order to deal with this table.

current currently internal future name future internal
qgis_detect_windows() 0 qgis_detect_windows_paths()
qgis_detect_macos() 0 qgis_detect_macos_paths()
is_windows() 0 1
is_macos() 0 1
qgis_has_plugin() 1 0
qgis_help() 0 qgis_help_json() 1
qgis_parsed_help() 1 qgis_parse_help()
qgis_use_cached_help() 1 qgis_using_cached_help()
qgis_use_json_input() 0 qgis_using_json_input()
qgis_use_json_output() 0 qgis_using_json_output()
qgis_description() 0 qgis_get_description()
qgis_arguments() 0 qgis_get_argument_specs()
qgis_outputs() 0 qgis_get_output_specs()
qgis_output() 0 qgis_extract_output_by_name() – but rework with by default the single OUTPUT element
qgis_result_single() 0 qgis_extract_output_by_class() – rework with only by default the single OUTPUT element
qgis_pipe() 0 qgis_run_algorithm_p()
qgis_sanitize_arguments() 0 1
qgis_argument_spec_by_name() 0 1
qgis_argument_spec() 0 1
is_gis_default_value() 0 1
qgis_default_value() 0 1
as_qgis_argument() + methods 0 0 needed for methods but don't show this one in package index (e.g. like various testthat functions)
qgis_clean_arguments() 0 1
qgis_clean_argument() 0 0 needed for methods but don't show this one in package index
assert_qgis_algorithm() 0 1
assert_qgis() 0 1
qgis_env() 0 1
is_qgis_tmp_file() 0 1
qgis_tmp_clean() 0 qgis_clean_tmp()
qgis_result_clean() 0 qgis_clean_result()
is_qgis_result() 0 1
qgis_error_output_does_not_exist() 0 1

Further renaming ideas to simplify function names

  • q_ instead of qgis_ prefix? Deprecate qgis_ names?

  • Add shorthand functions for some most used functions (not deprecating the long ones):

    long short
    qgis_run_algorithm() q_run_alg()
    qgis_run_algorithm_p() q_run_alg_p()

Function families (for the exported, indexed functions)

The proposed new function names are already implemented below. Changed names are marked with an asterisk (*). Italics are shorthand functions.

Functions to search QGIS installations in Windows and macOS

qgis_detect_windows_paths() *
qgis_detect_macos_paths() *

Functions to report about (cached) QGIS state

qgis_algorithms()
qgis_providers()
qgis_plugins()
qgis_path()
qgis_version()

Functions to manage plugins

qgis_enable_plugins()
qgis_disable_plugins()

Functions to check QGIS state and return a logical(1)

has_qgis()
qgis_has_plugin()
qgis_has_provider()
qgis_has_algorithm()

Functions to report about (cached) usage of JSON

qgis_using_json_output() *
qgis_using_json_input() *

Functions to inform about one algorithm

qgis_show_help()
qgis_get_description() *
qgis_get_argument_specs() *
qgis_get_output_specs() *

Function to create a wrapper for one algorithm

qgis_function()

Functions to run one algorithm

qgis_run_algorithm()
qgis_run_algorithm_p() *

Function to call the 'qgis_process' command directly

qgis_run()

Functions to access or manage processing output

qgis_extract_output_by_name() *
qgis_extract_output_by_class() *
qgis_clean_result() *

Functions to coerce processing output

st_as_sf()
qgis_as_terra()
st_as_stars()
qgis_as_raster()
qgis_as_brick()

Functions to create more rigid list input arguments for qgisprocess

qgis_list_input()
qgis_dict_input()

Functions to work with temporary file-based data

qgis_tmp_base()
qgis_tmp_folder()
qgis_tmp_vector()
qgis_tmp_raster()
qgis_tmp_file()
qgis_clean_tmp() *

Functions useful for debugging

qgis_unconfigure()
qgis_result_status()
qgis_result_stdout()
qgis_result_stderr()
qgis_result_args()

@florisvdh
Copy link
Member Author

Note to self:

Plotting function dependencies, dropping methods
# Plotting all function dependencies
library(DependenciesGraphs)
library(qgisprocess)
dep <- envirDependencies("package:qgisprocess")
plot(dep)

# Filter methods
library(dplyr)
library(stringr)
dep$Nomfun %>% 
  mutate(generic = str_sub(label, end = str_locate(label, "\\.")[, "start"] - 1)) %>% 
  filter(!is.na(generic))

# Change object and don't include methods in the plot
dep$Nomfun <-
  dep$Nomfun %>%
  filter(!str_detect(label, "\\."))
dep$fromto <- 
  semi_join(dep$fromto, dep$Nomfun %>% mutate(id2 = as.integer(id)), by = c("to" = "id2"))
plot(dep)

# Plot dependency graph of one function (also highlighting exported vs internal)
library(flow)
flow_view_deps(qgis_arguments)
flow_view_deps(qgis_run_algorithm)

@florisvdh
Copy link
Member Author

florisvdh commented Mar 15, 2023

Follow-up of a meeting with Dewey.

We keep as_qgis_argument() methods exported since it's needed for methods to do so. I'll hide them from the package index page though, in order not to overwhelm users.

qgis_unconfigure() and qgis_result_*() functions will remain exported, within an extra 'debugging' function family.

These things have been updated in the overview at the top.

Further, we'll keep the full qgis_ prefix since it has the advantage of being a good filter for the function dropdown in RStudio, while typing. So dropping the idea of some shorthand functions too.

@florisvdh
Copy link
Member Author

florisvdh commented Mar 29, 2023

Additionally, will also keep exporting qgis_list_input() and qgis_dict_input(). They are now enlisted above in their own function family. Reasons for exporting below.

  • qgis_list_input() is required for multilayer arguments in case of no-JSON-input. With JSON input, it works with both qgis_list_input() and list(). But qgis_list_input() applies more strict rules, i.e. must be unnamed, so is a more explicit approach.
Tried with example from #25
> qgis_use_json_input()
[1] FALSE
> rstr <- system.file("longlake/longlake_depth.tif", package = "qgisprocess")
> pts <- system.file("longlake/longlake_depth.gpkg", package = "qgisprocess")
> qgis_run_algorithm(
+   "sagang:addrastervaluestopoints",
+   SHAPES = pts,
+   GRIDS = qgis_list_input(rstr, rstr),
+   RESAMPLING = 0,
+   RESULT = qgis_tmp_vector()
+ )
Running /home/floris/git_repositories2/QGIS/build-4d78e34/output/bin/qgis_process --json run \
  'sagang:addrastervaluestopoints' \
  '--SHAPES=/media/floris/DATA/git_repositories/qgisprocess/inst/longlake/longlake_depth.gpkg' \
  '--GRIDS=/media/floris/DATA/git_repositories/qgisprocess/inst/longlake/longlake_depth.tif' \
  '--GRIDS=/media/floris/DATA/git_repositories/qgisprocess/inst/longlake/longlake_depth.tif' \
  '--RESULT=/tmp/Rtmph7uAYp/file2eb9b4938ac/file2eb96b47ad9.gpkg' '--RESAMPLING=0'

<Result of `qgis_run_algorithm("sagang:addrastervaluestopoints", ...)`>
List of 1
 $ RESULT: 'qgis_outputVector' chr "/tmp/Rtmph7uAYp/file2eb9b4938ac/file2eb96b47ad9.gpkg"
> qgis_run_algorithm(
+   "sagang:addrastervaluestopoints",
+   SHAPES = pts,
+   GRIDS = list(rstr, rstr),
+   RESAMPLING = 0,
+   RESULT = qgis_tmp_vector()
+ )
Error in `as_qgis_argument()` at qgisprocess/R/qgis-arguments.R:85:4:
! Don't know how to convert object of type 'list' to QGIS type 'multilayer'
Run `rlang::last_trace()` to see where the error occurred.
  • qgis_dict_input() is only supported in the JSON input method, where it can be interchanged with (a named) list(). It can (only) be used for arguments requiring named lists. The good thing of using it, beside the stricter rule (named list requirement), is that it can be actively captured and will lead to a meaningful error in case of no-JSON-input.

Refactoring of a native:aggregate example from #74 (comment) using qgis_list_input() and qgis_dict_input() instead of just list(list(...), list(...)):

qgis_run_algorithm(
  "native:aggregate",
  AGGREGATES = qgis_list_input(
    qgis_dict_input(
      aggregate = "first_value",
      delimiter = ",",
      input = '"admin"',
      length = 36,
      name = "admin",
      precision = 0,
      type = 10
    ),
    qgis_dict_input(
      aggregate = "concatenate",
      delimiter = ",",
      input = '"name"',
      length = 3000,
      name = "name",
      precision = 0,
      type = 10
    )
  ),
  INPUT = "world_map.gpkg|layername=states_provinces",
  GROUP_BY = "admin"
)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant