diff --git a/doc/syntax/clause/facet.qmd b/doc/syntax/clause/facet.qmd index 710fa48e..b1cce3cb 100644 --- a/doc/syntax/clause/facet.qmd +++ b/doc/syntax/clause/facet.qmd @@ -34,7 +34,7 @@ This clause behaves much like the `SETTINGS` clause in `DRAW`, in that it allows * `'y'`: Shared x-axis scale, independent y-axis scale * `['x', 'y']`: Independent scales for both axes * `missing`: Determines how layers behave when the faceting column is missing. It can take two values: `'repeat'` (default), and `'null'`. If `'repeat'` is set, then the layer data is repeated in each panel. If `'null'`, then such layers are only displayed if a null panel is shown, as controlled by the facet scale. -* `ncol`/`nrow`: The dimensions of the layout when faceting by a single variable. Only one of these can be given, as the other is derived based on the number of panels to draw. Default is 3 columns when fewer than 6 categories are present, 4 columns when fewer than 12 categories are present and otherwise 5 columns. When the `BY`-clause is used to set a second faceting variable, the `ncol` and `nrow` setting are not allowed. +* `ncol`/`nrow`: The dimensions of the layout when faceting by a single variable (whole number >= 1). Only one of these can be given, as the other is derived based on the number of panels to draw. Default is 3 columns when fewer than 6 categories are present, 4 columns when fewer than 12 categories are present and otherwise 5 columns. When the `BY`-clause is used to set a second faceting variable, the `ncol` and `nrow` setting are not allowed. ### Facet variables as aesthetics When you apply faceting to a plot you are creating new aesthetics you can control. For 1-dimensional faceting (no `BY` clause) the aesthetic is called `panel` and for 2-dimensional faceting the aesthetics are called `row` and `column`. You can read more about these aesthetics in [their documentation](../scale/aesthetic/Z_faceting.qmd) diff --git a/doc/syntax/coord/cartesian.qmd b/doc/syntax/coord/cartesian.qmd index c807b88d..3a84e66e 100644 --- a/doc/syntax/coord/cartesian.qmd +++ b/doc/syntax/coord/cartesian.qmd @@ -20,7 +20,7 @@ assuming they do not try to use a name that is already being used by any facet o ## Settings * `clip`: Should data be removed if it appears outside the bounds of the coordinate system. Defaults to `true` -* `ratio`: The aspect ratio between the steps on the vertical and horizontal axis. Defaults to `null` (no enforced aspect ratio) +* `ratio`: The aspect ratio between the steps on the vertical and horizontal axis (must be > 0 if specified). Defaults to `null` (no enforced aspect ratio) ## Examples diff --git a/doc/syntax/coord/polar.qmd b/doc/syntax/coord/polar.qmd index 348e33e7..3d9d6a4b 100644 --- a/doc/syntax/coord/polar.qmd +++ b/doc/syntax/coord/polar.qmd @@ -20,12 +20,12 @@ This maps `y` to radius and `x` to angle. This is useful when converting from a ## Settings * `clip`: Should data be removed if it appears outside the bounds of the coordinate system. Defaults to `true` -* `start`: The starting angle in degrees for the angle scale. Controls where "0" on the angular axis begins. Defaults to `0` (12 o'clock position). +* `start`: The starting angle in degrees for the theta scale (-360 to 360). Controls where "0" on the angular axis begins. Defaults to `0` (12 o'clock position). - `0` = 12 o'clock position (top) - `90` = 3 o'clock position (right) - `-90` or `270` = 9 o'clock position (left) - `180` = 6 o'clock position (bottom) -* `end`: The ending angle in degrees for the angle scale. Defaults to `start + 360` (a full circle). Use this with `start` to create partial polar plots like gauge charts or half-circle visualizations. +* `end`: The ending angle in degrees for the theta scale (-360 to 360). Defaults to `start + 360` (a full circle). Use this with `start` to create partial polar plots like gauge charts or half-circle visualizations. * `inner`: The inner radius as a proportion (0 to 1) of the outer radius. Defaults to `0` (no hole). Setting this creates a donut chart where the inner portion is empty. - `0` = full pie (no hole) - `0.3` = donut with 30% hole diff --git a/doc/syntax/layer/position/dodge.qmd b/doc/syntax/layer/position/dodge.qmd index e0a98a36..f3e8955a 100644 --- a/doc/syntax/layer/position/dodge.qmd +++ b/doc/syntax/layer/position/dodge.qmd @@ -12,7 +12,7 @@ Dodge doesn't have specific requirements to the scale type of the plot, but will ## Settings Apart from the settings of the layer type, setting `position => 'dodge'` will allow these additional settings: -* `width`: The total width the dodging will occupy as a proportion of the space available on the scale. Defaults to 0.9 but any defaults from the layer will take precedence. +* `width`: The total width the dodging will occupy as a proportion of the space available on the scale (0 to 1). Defaults to 0.9 but any defaults from the layer will take precedence. ## Examples diff --git a/doc/syntax/layer/position/jitter.qmd b/doc/syntax/layer/position/jitter.qmd index f30293ed..c6537c25 100644 --- a/doc/syntax/layer/position/jitter.qmd +++ b/doc/syntax/layer/position/jitter.qmd @@ -12,17 +12,17 @@ Jitter requires at least one axis to be discrete as it only jitters along discre ## Settings Apart from the settings of the layer type, setting `position => 'jitter'` will allow these additional settings: -* `width`: The total width the jittering will occupy as a proportion of the space available on the scale. Defaults to 0.9 +* `width`: The total width the jittering will occupy as a proportion of the space available on the scale (0 to 1). Defaults to 0.4 * `dodge`: Should dodging be applied before jittering. The dodging behavior follows the [dodge position](dodge.qmd) behavior? Default to `true` * `distribution`: Which kind of distribution should the jittering follow? One of: - `'uniform'` (default): Jittering is sampled from a uniform distribution between `-width/2` and `width/2` - `'normal'`: Jittering is sampled from a normal distribution with σ as `width/4` resulting in 95% of the points falling inside the given width - `'density'`: Jittering follows the density distribution within the group so that the jitter occupies the same area as an equivalent [violin plot](../type/violin.qmd) with density remapped to offset - `'intensity'`: Jittering follows the intensity distribution within the group so that the jitter occupies the same area as an equivalent [violin plot](../type/violin.qmd) with intensity remapped to offset - + If `distribution` is either `'density'` or `'intensity'` then one of the axes must be continuous -* `bandwidth`: A numerical value setting the smoothing bandwidth to use for the `'density'` and `'intensity'` distributions. If absent (default), the bandwidth will be computed using Silverman's rule of thumb. -* `adjust`: A numerical value as multiplier for the `bandwidth` setting, with 1 as default. +* `bandwidth`: Smoothing bandwidth for the `'density'` and `'intensity'` distributions (must be > 0). If absent (default), the bandwidth will be computed using Silverman's rule of thumb. +* `adjust`: Multiplier for the `bandwidth` setting (must be > 0). Defaults to 1. ## Examples When plotting points on a discrete axis they are all placed in the middle diff --git a/doc/syntax/layer/position/stack.qmd b/doc/syntax/layer/position/stack.qmd index 8f324660..761e5105 100644 --- a/doc/syntax/layer/position/stack.qmd +++ b/doc/syntax/layer/position/stack.qmd @@ -13,7 +13,7 @@ Stack requires a continuous scale with a range mapping (e.g. either `y` + `yend` Apart from the settings of the layer type, setting `position => 'stack'` will allow these additional settings: * `center`: Should the full stack be centered around 0. Can be used in conjunction with area layers to create steamgraphs. Default to `false` -* `total`: Sets a total value to which each stack height is normalised. Setting this value leads to 'fill' behaviour. Defaults to `null` (no normalisation) +* `total`: Sets a total value to which each stack height is normalised (must be > 0 if specified). Setting this value leads to 'fill' behaviour. Defaults to `null` (no normalisation) ## Examples diff --git a/doc/syntax/layer/type/area.qmd b/doc/syntax/layer/type/area.qmd index 8643e197..0d9a17a0 100644 --- a/doc/syntax/layer/type/area.qmd +++ b/doc/syntax/layer/type/area.qmd @@ -21,8 +21,8 @@ The following aesthetics are recognised by the area layer. * `linewidth`: The width of the contour lines. ## Settings -* `position`: Determines the position adjustment to use for the layer (default is `'stack'`) -* `orientation`: The orientation of the layer, see the [Orientation section](#orientation). One of the following: +* `position`: Position adjustment. One of `'identity'`, `'stack'` (default), `'dodge'`, or `'jitter'` +* `orientation`: The orientation of the layer, see the [Orientation section](#orientation). One of the following: * `'aligned'` to align the layer's primary axis with the coordinate system's first axis. * `'transposed'` to align the layer's primary axis with the coordinate system's second axis. diff --git a/doc/syntax/layer/type/bar.qmd b/doc/syntax/layer/type/bar.qmd index ca809bd8..089c89fa 100644 --- a/doc/syntax/layer/type/bar.qmd +++ b/doc/syntax/layer/type/bar.qmd @@ -23,8 +23,8 @@ The bar layer has no required aesthetics * `linetype`: The type of stroke, i.e. the dashing pattern ## Settings -* `position`: Determines the position adjustment to use for the layer (default is `'stack'`) -* `width`: The width of the bars as a proportion of the available width +* `position`: Position adjustment. One of `'identity'`, `'stack'` (default), `'dodge'`, or `'jitter'` +* `width`: The width of the bars as a proportion of the available width (0 to 1) ## Data transformation If the secondary axis has not been mapped the layer will calculate counts for you and display these as the secondary axis. diff --git a/doc/syntax/layer/type/boxplot.qmd b/doc/syntax/layer/type/boxplot.qmd index 93eddbc9..2363d278 100644 --- a/doc/syntax/layer/type/boxplot.qmd +++ b/doc/syntax/layer/type/boxplot.qmd @@ -23,10 +23,10 @@ The following aesthetics are recognised by the boxplot layer. * `shape` The shape of outlier points. ## Settings -* `position`: Determines the position adjustment to use for the layer (default is `'dodge'`) +* `position`: Position adjustment. One of `'identity'`, `'stack'`, `'dodge'` (default), or `'jitter'` * `outliers`: Whether to display outliers as points. Defaults to `true`. -* `coef`: A number indicating the length of the whiskers as a multiple of the interquartile range (IQR). Defaults to `1.5`. -* `width`: Relative width of the boxes. Defaults to `0.9`. +* `coef`: Length of the whiskers as a multiple of the IQR (must be >= 0). Defaults to `1.5`. +* `width`: Relative width of the boxes (0 to 1). Defaults to `0.9`. ## Data transformation Per group, data will be divided into 4 quartiles and summary statistics will be derived from their extremes. diff --git a/doc/syntax/layer/type/density.qmd b/doc/syntax/layer/type/density.qmd index 14853281..fe9f0c5a 100644 --- a/doc/syntax/layer/type/density.qmd +++ b/doc/syntax/layer/type/density.qmd @@ -21,9 +21,9 @@ The following aesthetics are recognised by the density layer. * `linetype` The dash pattern of the contour line. ## Settings -* `position`: Determines the position adjustment to use for the layer (default is `'identity'`) -* `bandwidth`: A numerical value setting the smoothing bandwidth to use. If absent (default), the bandwidth will be computed using Silverman's rule of thumb. -* `adjust`: A numerical value as multiplier for the `bandwidth` setting, with 1 as default. +* `position`: Position adjustment. One of `'identity'` (default), `'stack'`, `'dodge'`, or `'jitter'` +* `bandwidth`: Smoothing bandwidth (must be > 0). If absent (default), the bandwidth will be computed using Silverman's rule of thumb. +* `adjust`: Multiplier for the `bandwidth` setting (must be > 0). Defaults to 1. * `kernel`: Determines the smoothing kernel shape. Can be one of the following: * `'gaussian'` (default) * `'epanechnikov'` diff --git a/doc/syntax/layer/type/errorbar.qmd b/doc/syntax/layer/type/errorbar.qmd index a852bb20..57410be6 100644 --- a/doc/syntax/layer/type/errorbar.qmd +++ b/doc/syntax/layer/type/errorbar.qmd @@ -21,7 +21,7 @@ The following aesthetics are recognised by the errorbar layer. * `linetype`: The dash pattern of the lines in the errorbar. ## Settings -* `width`: The width of the hinges in points. Can be set to `null` to not display hinges. +* `width`: The width of the hinges in points (must be >= 0). Defaults to 10. Can be set to `null` to not display hinges. ## Data transformation The errorbar layer does not transform its data but passes it through unchanged. diff --git a/doc/syntax/layer/type/histogram.qmd b/doc/syntax/layer/type/histogram.qmd index 93c9ec57..efc00380 100644 --- a/doc/syntax/layer/type/histogram.qmd +++ b/doc/syntax/layer/type/histogram.qmd @@ -21,9 +21,9 @@ The following aesthetics are recognised by the bar layer. * `linetype`: The type of stroke, i.e. the dashing pattern ## Settings -* `position`: Determines the position adjustment to use for the layer (default is `'stack'`) -* `bins`: The number of bins to calculate. Defaults to `30` -* `binwidth`: The width of each bin. If provided it will override the binwidth calculated from `bins` +* `position`: Position adjustment. One of `'identity'`, `'stack'` (default), `'dodge'`, or `'jitter'` +* `bins`: The number of bins to calculate. Whole number >= 1. Defaults to `30` +* `binwidth`: The width of each bin (must be > 0). If provided it will override the binwidth calculated from `bins` * `closed`: Either `'left'` or `'right'` (default). Determines whether the bin intervals are closed to the left or right side ## Data transformation diff --git a/doc/syntax/layer/type/line.qmd b/doc/syntax/layer/type/line.qmd index e36319b1..853f8aca 100644 --- a/doc/syntax/layer/type/line.qmd +++ b/doc/syntax/layer/type/line.qmd @@ -20,8 +20,8 @@ The following aesthetics are recognised by the line layer. * `linetype`: The type of line, i.e. the dashing pattern ## Settings -* `position`: Determines the position adjustment to use for the layer (default is `'identity'`) -* `orientation`: The orientation of the layer, see the [Orientation section](#orientation). One of the following: +* `position`: Position adjustment. One of `'identity'` (default), `'stack'`, `'dodge'`, or `'jitter'` +* `orientation`: The orientation of the layer, see the [Orientation section](#orientation). One of the following: * `'aligned'` to align the layer's primary axis with the coordinate system's first axis. * `'transposed'` to align the layer's primary axis with the coordinate system's second axis. diff --git a/doc/syntax/layer/type/path.qmd b/doc/syntax/layer/type/path.qmd index cb58c4a1..03cdb0b9 100644 --- a/doc/syntax/layer/type/path.qmd +++ b/doc/syntax/layer/type/path.qmd @@ -20,7 +20,7 @@ The following aesthetics are recognised by the path layer. * `linetype`: The type of path, i.e. the dashing pattern ## Settings -* `position`: Determines the position adjustment to use for the layer (default is `'identity'`) +* `position`: Position adjustment. One of `'identity'` (default), `'stack'`, `'dodge'`, or `'jitter'` ## Data transformation The path layer does not transform its data but passes it through unchanged diff --git a/doc/syntax/layer/type/point.qmd b/doc/syntax/layer/type/point.qmd index 91b8af23..a413f7a3 100644 --- a/doc/syntax/layer/type/point.qmd +++ b/doc/syntax/layer/type/point.qmd @@ -22,7 +22,7 @@ The following aesthetics are recognised by the point layer. * `shape`: The shape used to draw the point ## Settings -* `position`: Determines the position adjustment to use for the layer (default is `'identity'`) +* `position`: Position adjustment. One of `'identity'` (default), `'stack'`, `'dodge'`, or `'jitter'` ## Data transformation The point layer does not transform its data but passes it through unchanged diff --git a/doc/syntax/layer/type/polygon.qmd b/doc/syntax/layer/type/polygon.qmd index 65d7ac72..cf234719 100644 --- a/doc/syntax/layer/type/polygon.qmd +++ b/doc/syntax/layer/type/polygon.qmd @@ -22,7 +22,7 @@ The following aesthetics are recognised by the polygon layer. * `linetype` The dash pattern of the contour line. ## Settings -* `position`: Determines the position adjustment to use for the layer (default is `'identity'`) +* `position`: Position adjustment. One of `'identity'` (default), `'stack'`, `'dodge'`, or `'jitter'` ## Data transformation The polygon layer does not transform its data but passes it through unchanged diff --git a/doc/syntax/layer/type/rect.qmd b/doc/syntax/layer/type/rect.qmd index 79134a45..977d9898 100644 --- a/doc/syntax/layer/type/rect.qmd +++ b/doc/syntax/layer/type/rect.qmd @@ -36,7 +36,7 @@ Alternatively, use only the center, which will set `height` to 1 by default. * `linetype`: The dash pattern of the contour line. ## Settings -* `position`: Determines the position adjustment to use for the layer (default is `'identity'`) +* `position`: Position adjustment. One of `'identity'` (default), `'stack'`, `'dodge'`, or `'jitter'` ## Data transformation. When the primary aesthetics are continuous, primary data is reparameterised to {start, end}, e.g. `xmin` and `xmax`. diff --git a/doc/syntax/layer/type/ribbon.qmd b/doc/syntax/layer/type/ribbon.qmd index 5b5e6d7d..0a72675b 100644 --- a/doc/syntax/layer/type/ribbon.qmd +++ b/doc/syntax/layer/type/ribbon.qmd @@ -22,7 +22,7 @@ The following aesthetics are recognised by the ribbon layer. * `linewidth`: The width of the contour lines. ## Settings -* `position`: Determines the position adjustment to use for the layer (default is `'identity'`) +* `position`: Position adjustment. One of `'identity'` (default), `'stack'`, `'dodge'`, or `'jitter'` ## Data transformation The ribbon layer sorts the data along its primary axis diff --git a/doc/syntax/layer/type/segment.qmd b/doc/syntax/layer/type/segment.qmd index fb5ef7fd..4cf627f6 100644 --- a/doc/syntax/layer/type/segment.qmd +++ b/doc/syntax/layer/type/segment.qmd @@ -25,7 +25,7 @@ If one is missing, it takes on the value of the start point. * `linetype`: The type of the line, i.e. the dashing pattern. ## Settings -* `position`: Determines the position adjustment to use for the layer (default is `'identity'`) +* `position`: Position adjustment. One of `'identity'` (default), `'stack'`, `'dodge'`, or `'jitter'` ## Data transformation The segment layer does not transform its data but passes it through unchanged. diff --git a/doc/syntax/layer/type/smooth.qmd b/doc/syntax/layer/type/smooth.qmd index b6e7b331..a7768aa0 100644 --- a/doc/syntax/layer/type/smooth.qmd +++ b/doc/syntax/layer/type/smooth.qmd @@ -26,8 +26,8 @@ Smooth layers are used to display a trendline among a series of observations. * `'tls'` estimates a straight trendline using total least squares method. The settings below only apply when `method => 'nw'` and are ignored when using other methods. -* `bandwidth`: A numerical value setting the smoothing bandwidth to use. If absent (default), the bandwidth will be computed using Silverman's rule of thumb. -* `adjust`: A numerical value as multiplier for the `bandwidth` setting, with 1 as default. +* `bandwidth`: Smoothing bandwidth (must be > 0). If absent (default), the bandwidth will be computed using Silverman's rule of thumb. +* `adjust`: Multiplier for the `bandwidth` setting (must be > 0). Defaults to 1. * `kernel`: Determines the smoothing kernel shape. Can be one of the following: * `'gaussian'` (default) * `'epanechnikov'` diff --git a/doc/syntax/layer/type/text.qmd b/doc/syntax/layer/type/text.qmd index 0e601c5b..059b68c8 100644 --- a/doc/syntax/layer/type/text.qmd +++ b/doc/syntax/layer/type/text.qmd @@ -32,9 +32,9 @@ The following aesthetics are recognised by the text layer. ## Settings * `offset` Position offset expressed in absolute points. Can be one of the following: * a single number that applies both horizontally and vertically - * an numeric array `[h, v]` where the first number is the horizontal offset and the second number is the vertical offset. + * a 2-element numeric array `[h, v]` where the first number is the horizontal offset and the second number is the vertical offset. * `format` Formatting specifier, see explanation below. -* `position`: Determines the position adjustment to use for the layer (default is `'identity'`) +* `position`: Position adjustment. One of `'identity'` (default), `'stack'`, `'dodge'`, or `'jitter'` ### Format The `format` setting can take a string that will be used in formatting the `label` aesthetic. diff --git a/doc/syntax/layer/type/violin.qmd b/doc/syntax/layer/type/violin.qmd index acc55eb8..311df0a3 100644 --- a/doc/syntax/layer/type/violin.qmd +++ b/doc/syntax/layer/type/violin.qmd @@ -23,9 +23,9 @@ The following aesthetics are recognised by the violin layer. * `linetype` The dash pattern of the contour line. ## Settings -* `position`: Determines the position adjustment to use for the layer (default is `'dodge'`) -* `bandwidth`: A numerical value setting the smoothing bandwidth to use. If absent (default), the bandwidth will be computed using Silverman's rule of thumb. -* `adjust`: A numerical value as multiplier for the `bandwidth` setting, with 1 as default. +* `position`: Position adjustment. One of `'identity'`, `'stack'`, `'dodge'` (default), or `'jitter'` +* `bandwidth`: Smoothing bandwidth (must be > 0). If absent (default), the bandwidth will be computed using Silverman's rule of thumb. +* `adjust`: Multiplier for the `bandwidth` setting (must be > 0). Defaults to 1. * `kernel`: Determines the smoothing kernel shape. Can be one of the following: * `'gaussian'` (default) * `'epanechnikov'` @@ -33,8 +33,8 @@ The following aesthetics are recognised by the violin layer. * `'rectangular'` or `'uniform'` * `'biweight'` or `'quartic'` * `'cosine'` -* `width`: Relative width of the violins. Defaults to `0.9`. -* `tails`: Expansion rule for drawing the tails. One of the following: +* `width`: Relative width of the violins (0 to 1). Defaults to `0.9`. +* `tails`: Expansion rule for drawing the tails (must be >= 0 if numeric). One of the following: * A number setting a multiple of adjusted bandwidths to expand each group's range. Defaults to 3. * `null` to use the whole data range rather than group ranges. diff --git a/doc/syntax/scale/type/binned.qmd b/doc/syntax/scale/type/binned.qmd index 414ab0a8..1ce36a3c 100644 --- a/doc/syntax/scale/type/binned.qmd +++ b/doc/syntax/scale/type/binned.qmd @@ -124,9 +124,9 @@ SCALE BINNED x VIA date ## Settings The following settings are recognised by binned scales: -* `expand` (only for `x`/`y`): Either a scalar number or 2-length array of numbers. Sets the expansion of the scale to either side of the range. If a scalar it gives the multiplicative expansion. If an array the first element is a multiplication factor and the second element is an additive constant. Defaults to `0.05` (5 %). Expansion is only applied to values that are not explicitly given by the user, i.e. if setting the range as `SCALE x FROM [0, null]` expansion will only be applied to the upper range. +* `expand` (only for `x`/`y`): Either a scalar number or 2-element array of numbers (values must be >= 0). Sets the expansion of the scale to either side of the range. If a scalar it gives the multiplicative expansion. If an array the first element is a multiplication factor and the second element is an additive constant. Defaults to `0.05` (5 %). Expansion is only applied to values that are not explicitly given by the user, i.e. if setting the range as `SCALE x FROM [0, null]` expansion will only be applied to the upper range. * `oob`: How should values outside of the scale input range be treated. One of `'censor'` (set to `null`), or `'squish'` (set to the nearest bin). Default is `'censor'`. When set to `'squish'` the terminal bin labels will be removed to reflect that they extend to -Inf and Inf. -* `breaks`: Either a scalar as described in [the section on breaks](#breaks), or an array of values to place breaks at. Defaults to `5`. +* `breaks`: Either a scalar (whole number >= 1) as described in [the section on breaks](#breaks), or an array of values to place breaks at. Defaults to `5`. * `pretty`: A boolean indicating which algorithm to use for automatic calculation of breaks as described in [the section on breaks](#breaks). Defaults to `true`. * `reverse`: A boolean indicating whether the scale direction should be reversed. Defaults to `false`. * `closed`: Either `'left'` or `'right'`. Determines which bin a value will be part of when it lies on the boundary. Defaults to `'left'` diff --git a/doc/syntax/scale/type/continuous.qmd b/doc/syntax/scale/type/continuous.qmd index bd2d3be4..dae25ed0 100644 --- a/doc/syntax/scale/type/continuous.qmd +++ b/doc/syntax/scale/type/continuous.qmd @@ -140,9 +140,9 @@ SCALE x ## Settings The following settings are recognised by continuous scales: -* `expand` (only for `x`/`y`): Either a scalar number or 2-length array of numbers. Sets the expansion of the scale to either side of the range. If a scalar it gives the multiplicative expansion. If an array the first element is a multiplication factor and the second element is an additive constant. Defaults to `0.05` (5 %). Expansion is only applied to values that are not explicitly given by the user, i.e. if setting the range as `SCALE x FROM [0, null]` expansion will only be applied to the upper range. +* `expand` (only for `x`/`y`): Either a scalar number or 2-element array of numbers (values must be >= 0). Sets the expansion of the scale to either side of the range. If a scalar it gives the multiplicative expansion. If an array the first element is a multiplication factor and the second element is an additive constant. Defaults to `0.05` (5 %). Expansion is only applied to values that are not explicitly given by the user, i.e. if setting the range as `SCALE x FROM [0, null]` expansion will only be applied to the upper range. * `oob`: How should values outside of the scale input range be treated. One of `'keep'` (keep the values as-is), `'censor'` (set to `null`), or `'squish'` (set to the nearest values within the range). Default for `x`/`y` is `'keep'`, for the remaining it is `'censor'`. -* `breaks`: Either a scalar as described in [the section on breaks](#breaks), or an array of values to place breaks at. Defaults to `5`. +* `breaks`: Either a scalar (whole number >= 1) as described in [the section on breaks](#breaks), or an array of values to place breaks at. Defaults to `5`. * `pretty`: A boolean indicating which algorithm to use for automatic calculation of breaks as described in [the section on breaks](#breaks). Defaults to `true`. * `reverse`: A boolean indicating whether the scale direction should be reversed. Defaults to `false`. diff --git a/src/execute/layer.rs b/src/execute/layer.rs index 1c67dce3..4993cf40 100644 --- a/src/execute/layer.rs +++ b/src/execute/layer.rs @@ -505,7 +505,7 @@ where // Apply literal default remappings from geom defaults (e.g., y2 => 0.0 for bar baseline). // These apply regardless of stat transform, but only if user hasn't overridden them. // Defaults are always in aligned orientation. - for (aesthetic, default_value) in layer.geom.default_remappings() { + for (aesthetic, default_value) in layer.geom.default_remappings().defaults { // Only process literal values here (Column values are handled in Transformed branch) if !matches!(default_value, DefaultAestheticValue::Column(_)) { // Only add if user hasn't already specified this aesthetic in remappings or mappings @@ -529,7 +529,7 @@ where // Build stat column -> aesthetic mappings from geom defaults for renaming let mut final_remappings: HashMap = HashMap::new(); - for (aesthetic, default_value) in layer.geom.default_remappings() { + for (aesthetic, default_value) in layer.geom.default_remappings().defaults { if let DefaultAestheticValue::Column(stat_col) = default_value { // Stat column mapping: stat_col -> aesthetic (for rename) final_remappings.insert(stat_col.to_string(), aesthetic.to_string()); diff --git a/src/execute/mod.rs b/src/execute/mod.rs index fc10d651..604dabef 100644 --- a/src/execute/mod.rs +++ b/src/execute/mod.rs @@ -133,7 +133,7 @@ fn validate( idx + 1, stat_col, layer.geom, - valid_stat_columns.join(", ") + crate::and_list(valid_stat_columns) ))); } } diff --git a/src/lib.rs b/src/lib.rs index 62761387..79f74959 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -40,6 +40,7 @@ pub mod format; pub mod naming; pub mod parser; pub mod plot; +pub mod util; pub mod reader; @@ -61,6 +62,9 @@ pub use plot::aesthetic::{ is_positional_aesthetic, AestheticContext, NON_POSITIONAL, POSITIONAL_SUFFIXES, }; +// Re-export string formatting utilities +pub use util::{and_list, and_list_quoted, or_list, or_list_quoted}; + // Future modules - not yet implemented // #[cfg(feature = "engine")] // pub mod engine; @@ -947,17 +951,17 @@ mod integration_tests { Err(e) => { let err = e.to_string(); assert!( - err.contains("Invalid setting 'orientation'"), + err.contains("not 'orientation'"), "Error should mention invalid setting: {}", err ); assert!( err.contains("bar"), - "Error should mention the geom type: {}", + "Error should mention the layer type: {}", err ); } - Ok(_) => panic!("Should reject orientation setting for bar geom"), + Ok(_) => panic!("Should reject orientation setting for bar layer"), } // 2. Point geom (symmetrical, no orientation concept) - should reject @@ -972,12 +976,12 @@ mod integration_tests { Err(e) => { let err2 = e.to_string(); assert!( - err2.contains("Invalid setting 'orientation'"), + err2.contains("not 'orientation'"), "Error should mention invalid setting: {}", err2 ); } - Ok(_) => panic!("Should reject orientation setting for point geom"), + Ok(_) => panic!("Should reject orientation setting for point layer"), } // 3. Line geom (has orientation in default_params) - should accept diff --git a/src/parser/builder.rs b/src/parser/builder.rs index 9b889ca1..2c9d17fd 100644 --- a/src/parser/builder.rs +++ b/src/parser/builder.rs @@ -814,7 +814,7 @@ fn parse_scale_via_clause(node: &Node, source: &SourceTree) -> Result GgsqlError::ParseError(format!( "Unknown transform: '{}'. Valid transforms are: {}", transform_name, - crate::plot::scale::ALL_TRANSFORM_NAMES.join(", ") + crate::and_list_quoted(crate::plot::scale::ALL_TRANSFORM_NAMES, '\'') )) }) } diff --git a/src/plot/facet/resolve.rs b/src/plot/facet/resolve.rs index 777e515d..5c9808a9 100644 --- a/src/plot/facet/resolve.rs +++ b/src/plot/facet/resolve.rs @@ -2,6 +2,7 @@ //! //! Validates facet properties and applies data-aware defaults. +use crate::plot::types::validate_parameter; use crate::plot::ParameterValue; use crate::DataFrame; use std::collections::HashMap; @@ -50,15 +51,6 @@ impl FacetDataContext { } } -/// Allowed properties for wrap facets -const WRAP_ALLOWED: &[&str] = &["free", "ncol", "nrow", "missing"]; - -/// Allowed properties for grid facets -const GRID_ALLOWED: &[&str] = &["free", "missing"]; - -/// Valid values for the missing property -const MISSING_VALUES: &[&str] = &["repeat", "null"]; - /// Compute smart default ncol for wrap facets based on number of levels /// /// Returns an optimal column count that creates a balanced grid: @@ -83,13 +75,13 @@ fn compute_default_ncol(num_levels: usize) -> i64 { /// This function: /// 1. Skips if already resolved /// 2. Validates all properties are allowed for this layout -/// 3. Validates property values: -/// - `free`: must be null, a valid positional aesthetic, or an array of them -/// - `ncol`: positive integer -/// 4. Normalizes the `free` property to a boolean vector (position-indexed) -/// 5. Applies defaults for missing properties: +/// 3. Validates property values using constraints from `default_properties()` +/// 4. Validates `free` property against positional aesthetic names (coord-dependent) +/// 5. Validates ncol/nrow mutual exclusivity +/// 6. Normalizes the `free` property to a boolean vector (position-indexed) +/// 7. Applies defaults for missing properties: /// - `ncol` (wrap only): computed from `context.num_levels` -/// 6. Sets `resolved = true` +/// 8. Sets `resolved = true` /// /// # Arguments /// @@ -106,39 +98,46 @@ pub fn resolve_properties( return Ok(()); } - let is_wrap = facet.is_wrap(); + let defaults = facet.layout.default_properties(); - // Step 1: Validate all properties are allowed for this layout - let allowed = if is_wrap { WRAP_ALLOWED } else { GRID_ALLOWED }; - for key in facet.properties.keys() { - if !allowed.contains(&key.as_str()) { - if key == "ncol" && !is_wrap { + // Step 1: Validate all properties are allowed and validate their values + for (key, value) in facet.properties.iter() { + if let Some(param) = defaults.iter().find(|p| p.name == key) { + // Skip validation for 'free' - it has coord-dependent allowed values + if key != "free" { + validate_parameter(key, value, ¶m.constraint)?; + } + } else { + // Special error messages for ncol/nrow on grid facets + if key == "ncol" { return Err( - "Setting `ncol` is only allowed for 1 dimensional facets, not 2 dimensional facets".to_string(), + "Setting 'ncol' is only allowed for 1 dimensional facets, not 2 dimensional facets".to_string(), ); } - if key == "nrow" && !is_wrap { + if key == "nrow" { return Err( - "Setting `nrow` is only allowed for 1 dimensional facets, not 2 dimensional facets".to_string(), + "Setting 'nrow' is only allowed for 1 dimensional facets, not 2 dimensional facets".to_string(), ); } + let allowed: Vec<&str> = defaults.iter().map(|p| p.name).collect(); return Err(format!( - "Unknown setting: '{}'. Allowed settings: {}", - key, - allowed.join(", ") + "FACET setting should be {}, not '{}'", + crate::or_list_quoted(&allowed, '\''), + key )); } } - // Step 2: Validate property values + // Step 2: Validate free property against coord-dependent positional names validate_free_property(facet, positional_names)?; - validate_layout_properties(facet)?; - validate_missing_property(facet)?; - // Step 3: Normalize free property to boolean vector + // Step 3: Validate ncol/nrow mutual exclusivity + validate_layout_exclusivity(facet)?; + + // Step 4: Normalize free property to boolean vector normalize_free_property(facet, positional_names); - // Step 4: Apply defaults for missing properties + // Step 5: Apply defaults for missing properties apply_defaults(facet, context); // Mark as resolved @@ -168,9 +167,9 @@ fn validate_free_property(facet: &Facet, positional_names: &[&str]) -> Result<() ParameterValue::String(s) => { if !positional_names.contains(&s.as_str()) { return Err(format!( - "invalid 'free' value '{}'. Expected one of: {}, or null", + "invalid 'free' value '{}'. Expected one of: {} (or null)", s, - format_options(positional_names) + crate::or_list_quoted(positional_names, '\'') )); } Ok(()) @@ -196,7 +195,7 @@ fn validate_free_property(facet: &Facet, positional_names: &[&str]) -> Result<() return Err(format!( "invalid 'free' array element '{}'. Expected one of: {}", s, - format_options(positional_names) + crate::or_list_quoted(positional_names, '\'') )); } if !seen.insert(s.clone()) { @@ -209,7 +208,7 @@ fn validate_free_property(facet: &Facet, positional_names: &[&str]) -> Result<() _ => { return Err(format!( "invalid 'free' array: elements must be strings. Expected: {}", - format_options(positional_names) + crate::or_list_quoted(positional_names, '\'') )); } } @@ -218,7 +217,7 @@ fn validate_free_property(facet: &Facet, positional_names: &[&str]) -> Result<() } _ => Err(format!( "'free' must be null, a string ({}), or an array of positional names", - format_options(positional_names) + crate::or_list_quoted(positional_names, '\'') )), } } else { @@ -226,15 +225,6 @@ fn validate_free_property(facet: &Facet, positional_names: &[&str]) -> Result<() } } -/// Format positional names for error messages -fn format_options(names: &[&str]) -> String { - names - .iter() - .map(|n| format!("'{}'", n)) - .collect::>() - .join(", ") -} - /// Normalize free property to a boolean vector /// /// Transforms user-provided values to a boolean vector (position-indexed): @@ -284,66 +274,20 @@ fn normalize_free_property(facet: &mut Facet, positional_names: &[&str]) { .insert("free".to_string(), ParameterValue::Array(bool_array)); } -/// Validate ncol and nrow properties +/// Validate ncol and nrow mutual exclusivity /// -/// - Both must be positive integers if present -/// - They are mutually exclusive (cannot both be specified) -fn validate_layout_properties(facet: &Facet) -> Result<(), String> { +/// They cannot both be specified at the same time. +/// Type and range validation is handled by the constraint system. +fn validate_layout_exclusivity(facet: &Facet) -> Result<(), String> { let has_ncol = facet.properties.contains_key("ncol"); let has_nrow = facet.properties.contains_key("nrow"); - // Check mutual exclusivity first if has_ncol && has_nrow { return Err( - "`ncol` and `nrow` cannot both be specified. Use one or the other.".to_string(), + "'ncol' and 'nrow' cannot both be specified. Use one or the other.".to_string(), ); } - // Validate ncol if present - if let Some(value) = facet.properties.get("ncol") { - match value { - ParameterValue::Number(n) => { - if *n <= 0.0 || n.fract() != 0.0 { - return Err(format!("`ncol` must be a positive integer, got {}", n)); - } - } - _ => return Err("'ncol' must be a number".to_string()), - } - } - - // Validate nrow if present - if let Some(value) = facet.properties.get("nrow") { - match value { - ParameterValue::Number(n) => { - if *n <= 0.0 || n.fract() != 0.0 { - return Err(format!("`nrow` must be a positive integer, got {}", n)); - } - } - _ => return Err("'nrow' must be a number".to_string()), - } - } - - Ok(()) -} - -/// Validate missing property value -fn validate_missing_property(facet: &Facet) -> Result<(), String> { - if let Some(value) = facet.properties.get("missing") { - match value { - ParameterValue::String(s) => { - if !MISSING_VALUES.contains(&s.as_str()) { - return Err(format!( - "invalid 'missing' value '{}'. Expected one of: {}", - s, - MISSING_VALUES.join(", ") - )); - } - } - _ => { - return Err("'missing' must be a string ('repeat' or 'null')".to_string()); - } - } - } Ok(()) } @@ -503,8 +447,7 @@ mod tests { assert!(result.is_err()); let err = result.unwrap_err(); - assert!(err.contains("Unknown setting")); - assert!(err.contains("columns")); + assert!(err.contains("not 'columns'")); } #[test] @@ -535,7 +478,7 @@ mod tests { assert!(result.is_err()); let err = result.unwrap_err(); - assert!(err.contains("Unknown setting")); + assert!(err.contains("not 'unknown'")); } #[test] @@ -568,7 +511,7 @@ mod tests { assert!(result.is_err()); let err = result.unwrap_err(); assert!(err.contains("ncol")); - assert!(err.contains("positive")); + assert!(err.contains(">= 1")); // count constraint: >= 1 } #[test] @@ -584,7 +527,7 @@ mod tests { assert!(result.is_err()); let err = result.unwrap_err(); assert!(err.contains("ncol")); - assert!(err.contains("integer")); + assert!(err.contains("whole number")); // count constraint checks for whole numbers } #[test] @@ -695,7 +638,7 @@ mod tests { assert!(result.is_err()); let err = result.unwrap_err(); assert!(err.contains("missing")); - assert!(err.contains("string")); + assert!(err.contains("String")); // type error uses capitalized type names } #[test] @@ -1060,7 +1003,7 @@ mod tests { assert!(result.is_err()); let err = result.unwrap_err(); assert!(err.contains("nrow")); - assert!(err.contains("positive")); + assert!(err.contains(">= 1")); // count constraint: >= 1 } #[test] @@ -1076,7 +1019,7 @@ mod tests { assert!(result.is_err()); let err = result.unwrap_err(); assert!(err.contains("nrow")); - assert!(err.contains("integer")); + assert!(err.contains("whole number")); // count constraint checks for whole numbers } #[test] @@ -1108,7 +1051,7 @@ mod tests { assert!(result.is_err()); let err = result.unwrap_err(); assert!(err.contains("nrow")); - assert!(err.contains("number")); + assert!(err.contains("Number")); // type error uses capitalized type names } #[test] diff --git a/src/plot/facet/types.rs b/src/plot/facet/types.rs index ce2cee5c..51052b9f 100644 --- a/src/plot/facet/types.rs +++ b/src/plot/facet/types.rs @@ -2,6 +2,7 @@ //! //! This module defines faceting configuration for small multiples. +use crate::plot::types::{ParamConstraint, ParamDefinition, DefaultParamValue}; use crate::plot::ParameterValue; use serde::{Deserialize, Serialize}; use std::collections::HashMap; @@ -167,4 +168,56 @@ impl FacetLayout { } } } + + /// Returns the default properties for this facet layout type. + /// + /// Wrap facets support: free, ncol, nrow, missing + /// Grid facets support: free, missing + pub fn default_properties(&self) -> &'static [ParamDefinition] { + /// Valid values for the missing property + const MISSING_VALUES: &[&str] = &["repeat", "null"]; + + match self { + FacetLayout::Wrap { .. } => { + const WRAP_PARAMS: &[ParamDefinition] = &[ + ParamDefinition { + name: "free", + default: DefaultParamValue::Null, + constraint: ParamConstraint::unconstrained(), // Validated separately due to coord-dependent values + }, + ParamDefinition { + name: "ncol", + default: DefaultParamValue::Null, // Computed from data + constraint: ParamConstraint::count(1.0), + }, + ParamDefinition { + name: "nrow", + default: DefaultParamValue::Null, + constraint: ParamConstraint::count(1.0), + }, + ParamDefinition { + name: "missing", + default: DefaultParamValue::Null, + constraint: ParamConstraint::string_option(MISSING_VALUES), + }, + ]; + WRAP_PARAMS + } + FacetLayout::Grid { .. } => { + const GRID_PARAMS: &[ParamDefinition] = &[ + ParamDefinition { + name: "free", + default: DefaultParamValue::Null, + constraint: ParamConstraint::unconstrained(), // Validated separately due to coord-dependent values + }, + ParamDefinition { + name: "missing", + default: DefaultParamValue::Null, + constraint: ParamConstraint::string_option(MISSING_VALUES), + }, + ]; + GRID_PARAMS + } + } + } } diff --git a/src/plot/layer/geom/area.rs b/src/plot/layer/geom/area.rs index ee660892..d9c456d9 100644 --- a/src/plot/layer/geom/area.rs +++ b/src/plot/layer/geom/area.rs @@ -1,9 +1,11 @@ //! Area geom implementation -use crate::plot::layer::orientation::ALIGNED; -use crate::plot::{types::DefaultAestheticValue, DefaultParam, DefaultParamValue}; +use crate::plot::layer::orientation::{ALIGNED, ORIENTATION_VALUES}; +use crate::plot::types::DefaultAestheticValue; +use crate::plot::{ParamDefinition, DefaultParamValue}; use crate::{naming, Mappings}; +use super::types::{ParamConstraint, POSITION_VALUES}; use super::{DefaultAesthetics, GeomTrait, GeomType, StatResult}; /// Area geom - filled area charts @@ -30,21 +32,26 @@ impl GeomTrait for Area { } } - fn default_remappings(&self) -> &'static [(&'static str, DefaultAestheticValue)] { - &[("pos2end", DefaultAestheticValue::Number(0.0))] + fn default_remappings(&self) -> DefaultAesthetics { + DefaultAesthetics { + defaults: &[("pos2end", DefaultAestheticValue::Number(0.0))], + } } - fn default_params(&self) -> &'static [DefaultParam] { - &[ - DefaultParam { + fn default_params(&self) -> &'static [ParamDefinition] { + const PARAMS: &[ParamDefinition] = &[ + ParamDefinition { name: "position", default: DefaultParamValue::String("stack"), + constraint: ParamConstraint::string_option(POSITION_VALUES), }, - DefaultParam { + ParamDefinition { name: "orientation", default: DefaultParamValue::String(ALIGNED), + constraint: ParamConstraint::string_option(ORIENTATION_VALUES), }, - ] + ]; + PARAMS } fn needs_stat_transform(&self, _aesthetics: &Mappings) -> bool { diff --git a/src/plot/layer/geom/arrow.rs b/src/plot/layer/geom/arrow.rs index 538b0446..75b0a750 100644 --- a/src/plot/layer/geom/arrow.rs +++ b/src/plot/layer/geom/arrow.rs @@ -1,6 +1,9 @@ //! Arrow geom implementation -use super::{DefaultAesthetics, DefaultParam, DefaultParamValue, GeomTrait, GeomType}; +use super::types::POSITION_VALUES; +use super::{ + DefaultAesthetics, GeomTrait, GeomType, ParamConstraint, ParamDefinition, DefaultParamValue, +}; use crate::plot::types::DefaultAestheticValue; /// Arrow geom - line segments with arrowheads @@ -28,11 +31,13 @@ impl GeomTrait for Arrow { } } - fn default_params(&self) -> &'static [DefaultParam] { - &[DefaultParam { + fn default_params(&self) -> &'static [ParamDefinition] { + const PARAMS: &[ParamDefinition] = &[ParamDefinition { name: "position", default: DefaultParamValue::String("identity"), - }] + constraint: ParamConstraint::string_option(POSITION_VALUES), + }]; + PARAMS } } diff --git a/src/plot/layer/geom/bar.rs b/src/plot/layer/geom/bar.rs index 5f246687..2cb5ec28 100644 --- a/src/plot/layer/geom/bar.rs +++ b/src/plot/layer/geom/bar.rs @@ -3,8 +3,11 @@ use std::collections::HashMap; use std::collections::HashSet; -use super::types::get_column_name; -use super::{DefaultAesthetics, DefaultParam, DefaultParamValue, GeomTrait, GeomType, StatResult}; +use super::types::{get_column_name, POSITION_VALUES}; +use super::{ + DefaultAesthetics, GeomTrait, GeomType, ParamConstraint, ParamDefinition, DefaultParamValue, + StatResult, +}; use crate::naming; use crate::plot::types::{DefaultAestheticValue, ParameterValue}; use crate::reader::SqlDialect; @@ -42,29 +45,34 @@ impl GeomTrait for Bar { } } - fn default_remappings(&self) -> &'static [(&'static str, DefaultAestheticValue)] { - &[ - ("pos2", DefaultAestheticValue::Column("count")), - ("pos1", DefaultAestheticValue::Column("pos1")), - ("pos2end", DefaultAestheticValue::Number(0.0)), - ] + fn default_remappings(&self) -> DefaultAesthetics { + DefaultAesthetics { + defaults: &[ + ("pos2", DefaultAestheticValue::Column("count")), + ("pos1", DefaultAestheticValue::Column("pos1")), + ("pos2end", DefaultAestheticValue::Number(0.0)), + ], + } } fn valid_stat_columns(&self) -> &'static [&'static str] { &["count", "pos1", "proportion"] } - fn default_params(&self) -> &'static [DefaultParam] { - &[ - DefaultParam { + fn default_params(&self) -> &'static [ParamDefinition] { + const PARAMS: &[ParamDefinition] = &[ + ParamDefinition { name: "width", default: DefaultParamValue::Number(0.9), + constraint: ParamConstraint::number_range(0.0, 1.0), }, - DefaultParam { + ParamDefinition { name: "position", default: DefaultParamValue::String("stack"), + constraint: ParamConstraint::string_option(POSITION_VALUES), }, - ] + ]; + PARAMS } fn stat_consumed_aesthetics(&self) -> &'static [&'static str] { diff --git a/src/plot/layer/geom/boxplot.rs b/src/plot/layer/geom/boxplot.rs index d94eef0a..8abdd4e0 100644 --- a/src/plot/layer/geom/boxplot.rs +++ b/src/plot/layer/geom/boxplot.rs @@ -2,12 +2,13 @@ use std::collections::HashMap; +use super::types::POSITION_VALUES; use super::{DefaultAesthetics, GeomTrait, GeomType}; use crate::{ naming, plot::{ - geom::types::get_column_name, DefaultAestheticValue, DefaultParam, DefaultParamValue, - ParameterValue, StatResult, + geom::types::get_column_name, DefaultAestheticValue, ParamConstraint, ParamDefinition, + DefaultParamValue, ParameterValue, StatResult, }, reader::SqlDialect, DataFrame, GgsqlError, Mappings, Result, @@ -49,33 +50,40 @@ impl GeomTrait for Boxplot { true } - fn default_params(&self) -> &'static [super::DefaultParam] { - &[ - DefaultParam { + fn default_params(&self) -> &'static [super::ParamDefinition] { + const PARAMS: &[ParamDefinition] = &[ + ParamDefinition { name: "outliers", - default: super::DefaultParamValue::Boolean(true), + default: DefaultParamValue::Boolean(true), + constraint: ParamConstraint::boolean(), }, - DefaultParam { + ParamDefinition { name: "coef", default: DefaultParamValue::Number(1.5), + constraint: ParamConstraint::number_min(0.0), }, - DefaultParam { + ParamDefinition { name: "width", default: DefaultParamValue::Number(0.9), + constraint: ParamConstraint::number_range(0.0, 1.0), }, - DefaultParam { + ParamDefinition { name: "position", default: DefaultParamValue::String("dodge"), + constraint: ParamConstraint::string_option(POSITION_VALUES), }, - ] + ]; + PARAMS } - fn default_remappings(&self) -> &'static [(&'static str, DefaultAestheticValue)] { - &[ - ("pos2", DefaultAestheticValue::Column("value")), - ("pos2end", DefaultAestheticValue::Column("value2")), - ("type", DefaultAestheticValue::Column("type")), - ] + fn default_remappings(&self) -> DefaultAesthetics { + DefaultAesthetics { + defaults: &[ + ("pos2", DefaultAestheticValue::Column("value")), + ("pos2end", DefaultAestheticValue::Column("value2")), + ("type", DefaultAestheticValue::Column("type")), + ], + } } fn apply_stat_transform( @@ -112,24 +120,14 @@ fn stat_boxplot( GgsqlError::ValidationError("Boxplot requires 'x' aesthetic mapping".to_string()) })?; - // Fetch coef parameter - let coef = match parameters.get("coef") { - Some(ParameterValue::Number(num)) => num, - _ => { - return Err(GgsqlError::InternalError( - "The 'coef' boxplot parameter must be a numeric value.".to_string(), - )) - } + // Get coef parameter (validated by ParamConstraint::number_min) + let ParameterValue::Number(coef) = parameters.get("coef").unwrap() else { + unreachable!("coef validated by ParamConstraint::number_min") }; - // Fetch outliers parameter - let outliers = match parameters.get("outliers") { - Some(ParameterValue::Boolean(draw)) => draw, - _ => { - return Err(GgsqlError::InternalError( - "The 'outliers' parameter must be `true` or `false`.".to_string(), - )) - } + // Get outliers parameter (validated by ParamConstraint::boolean) + let ParameterValue::Boolean(outliers) = parameters.get("outliers").unwrap() else { + unreachable!("outliers validated by ParamConstraint::boolean") }; // Fix boxplots to be vertical, when we later have orientation this may change @@ -300,24 +298,8 @@ fn boxplot_sql_append_outliers( #[cfg(test)] mod tests { use super::*; - use crate::plot::AestheticValue; use crate::reader::AnsiDialect; - // ==================== Helper Functions ==================== - - fn create_basic_aesthetics() -> Mappings { - let mut aesthetics = Mappings::new(); - aesthetics.insert( - "pos1".to_string(), - AestheticValue::standard_column("category".to_string()), - ); - aesthetics.insert( - "pos2".to_string(), - AestheticValue::standard_column("value".to_string()), - ); - aesthetics - } - // ==================== SQL Generation Tests (Compact) ==================== #[test] @@ -514,97 +496,6 @@ mod tests { assert!(outlier_section.contains("raw.year = summary.year")); } - // ==================== Parameter Validation Tests ==================== - - #[test] - fn test_stat_boxplot_invalid_coef_type() { - let aesthetics = create_basic_aesthetics(); - let groups = vec![]; - - let mut parameters = HashMap::new(); - parameters.insert( - "coef".to_string(), - ParameterValue::String("invalid".to_string()), - ); - parameters.insert("outliers".to_string(), ParameterValue::Boolean(true)); - - let result = stat_boxplot( - "SELECT * FROM data", - &aesthetics, - &groups, - ¶meters, - &AnsiDialect, - ); - - assert!(result.is_err()); - assert!(result.unwrap_err().to_string().contains("coef")); - } - - #[test] - fn test_stat_boxplot_missing_coef() { - let aesthetics = create_basic_aesthetics(); - let groups = vec![]; - - let mut parameters = HashMap::new(); - parameters.insert("outliers".to_string(), ParameterValue::Boolean(true)); - // Missing coef - - let result = stat_boxplot( - "SELECT * FROM data", - &aesthetics, - &groups, - ¶meters, - &AnsiDialect, - ); - - assert!(result.is_err()); - assert!(result.unwrap_err().to_string().contains("coef")); - } - - #[test] - fn test_stat_boxplot_invalid_outliers_type() { - let aesthetics = create_basic_aesthetics(); - let groups = vec![]; - - let mut parameters = HashMap::new(); - parameters.insert("coef".to_string(), ParameterValue::Number(1.5)); - parameters.insert( - "outliers".to_string(), - ParameterValue::String("yes".to_string()), - ); - - let result = stat_boxplot( - "SELECT * FROM data", - &aesthetics, - &groups, - ¶meters, - &AnsiDialect, - ); - - assert!(result.is_err()); - assert!(result.unwrap_err().to_string().contains("outliers")); - } - - #[test] - fn test_stat_boxplot_missing_outliers() { - let aesthetics = create_basic_aesthetics(); - let groups = vec![]; - - let mut parameters = HashMap::new(); - parameters.insert("coef".to_string(), ParameterValue::Number(1.5)); - // Missing outliers - - let result = stat_boxplot( - "SELECT * FROM data", - &aesthetics, - &groups, - ¶meters, - &AnsiDialect, - ); - - assert!(result.is_err()); - assert!(result.unwrap_err().to_string().contains("outliers")); - } // ==================== GeomTrait Implementation Tests ==================== @@ -677,10 +568,16 @@ mod tests { let boxplot = Boxplot; let remappings = boxplot.default_remappings(); - assert_eq!(remappings.len(), 3); - assert!(remappings.contains(&("pos2", DefaultAestheticValue::Column("value")))); - assert!(remappings.contains(&("pos2end", DefaultAestheticValue::Column("value2")))); - assert!(remappings.contains(&("type", DefaultAestheticValue::Column("type")))); + assert_eq!(remappings.defaults.len(), 3); + assert!(remappings + .defaults + .contains(&("pos2", DefaultAestheticValue::Column("value")))); + assert!(remappings + .defaults + .contains(&("pos2end", DefaultAestheticValue::Column("value2")))); + assert!(remappings + .defaults + .contains(&("type", DefaultAestheticValue::Column("type")))); } #[test] diff --git a/src/plot/layer/geom/density.rs b/src/plot/layer/geom/density.rs index 1d13bed3..851ab49e 100644 --- a/src/plot/layer/geom/density.rs +++ b/src/plot/layer/geom/density.rs @@ -1,11 +1,12 @@ //! Density geom implementation +use super::types::POSITION_VALUES; use super::{DefaultAesthetics, GeomTrait, GeomType}; use crate::{ naming, plot::{ - geom::types::get_column_name, DefaultAestheticValue, DefaultParam, DefaultParamValue, - ParameterValue, StatResult, + geom::types::get_column_name, DefaultAestheticValue, ParamConstraint, ParamDefinition, + DefaultParamValue, ParameterValue, StatResult, }, reader::SqlDialect, GgsqlError, Mappings, Result, @@ -16,6 +17,18 @@ use std::collections::HashMap; /// Precomputed at compile time to avoid repeated SQRT and PI() calls in SQL const GAUSSIAN_NORM: f64 = 0.3989422804014327; // 1.0 / (2.0 * std::f64::consts::PI).sqrt() +/// Valid kernel types for density estimation +const KERNEL_VALUES: &[&str] = &[ + "gaussian", + "epanechnikov", + "triangular", + "rectangular", + "uniform", + "biweight", + "quartic", + "cosine", +]; + /// Density geom - kernel density estimation #[derive(Debug, Clone, Copy)] pub struct Density; @@ -45,33 +58,40 @@ impl GeomTrait for Density { true } - fn default_params(&self) -> &'static [DefaultParam] { - &[ - DefaultParam { + fn default_params(&self) -> &'static [ParamDefinition] { + const PARAMS: &[ParamDefinition] = &[ + ParamDefinition { name: "position", default: DefaultParamValue::String("identity"), + constraint: ParamConstraint::string_option(POSITION_VALUES), }, - DefaultParam { + ParamDefinition { name: "bandwidth", default: DefaultParamValue::Null, + constraint: ParamConstraint::number_min_exclusive(0.0), }, - DefaultParam { + ParamDefinition { name: "adjust", default: DefaultParamValue::Number(1.0), + constraint: ParamConstraint::number_min_exclusive(0.0), }, - DefaultParam { + ParamDefinition { name: "kernel", default: DefaultParamValue::String("gaussian"), + constraint: ParamConstraint::string_option(KERNEL_VALUES), }, - ] + ]; + PARAMS } - fn default_remappings(&self) -> &'static [(&'static str, DefaultAestheticValue)] { - &[ - ("pos1", DefaultAestheticValue::Column("pos1")), - ("pos2", DefaultAestheticValue::Column("density")), - ("pos2end", DefaultAestheticValue::Number(0.0)), - ] + fn default_remappings(&self) -> DefaultAesthetics { + DefaultAesthetics { + defaults: &[ + ("pos1", DefaultAestheticValue::Column("pos1")), + ("pos2", DefaultAestheticValue::Column("density")), + ("pos2end", DefaultAestheticValue::Number(0.0)), + ], + } } fn valid_stat_columns(&self) -> &'static [&'static str] { diff --git a/src/plot/layer/geom/errorbar.rs b/src/plot/layer/geom/errorbar.rs index 18526303..f26fb464 100644 --- a/src/plot/layer/geom/errorbar.rs +++ b/src/plot/layer/geom/errorbar.rs @@ -1,6 +1,9 @@ //! ErrorBar geom implementation -use super::{DefaultAesthetics, DefaultParam, DefaultParamValue, GeomTrait, GeomType}; +use super::types::POSITION_VALUES; +use super::{ + DefaultAesthetics, GeomTrait, GeomType, ParamConstraint, ParamDefinition, DefaultParamValue, +}; use crate::plot::types::DefaultAestheticValue; /// ErrorBar geom - error bars (confidence intervals) @@ -29,17 +32,20 @@ impl GeomTrait for ErrorBar { } } - fn default_params(&self) -> &'static [DefaultParam] { - &[ - DefaultParam { + fn default_params(&self) -> &'static [ParamDefinition] { + const PARAMS: &[ParamDefinition] = &[ + ParamDefinition { name: "position", default: DefaultParamValue::String("identity"), + constraint: ParamConstraint::string_option(POSITION_VALUES), }, - DefaultParam { + ParamDefinition { name: "width", default: DefaultParamValue::Number(10.0), + constraint: ParamConstraint::number_min(0.0), }, - ] + ]; + PARAMS } } diff --git a/src/plot/layer/geom/histogram.rs b/src/plot/layer/geom/histogram.rs index b979b68c..ad6b60ac 100644 --- a/src/plot/layer/geom/histogram.rs +++ b/src/plot/layer/geom/histogram.rs @@ -2,8 +2,11 @@ use std::collections::HashMap; -use super::types::get_column_name; -use super::{DefaultAesthetics, DefaultParam, DefaultParamValue, GeomTrait, GeomType, StatResult}; +use super::types::{get_column_name, CLOSED_VALUES, POSITION_VALUES}; +use super::{ + DefaultAesthetics, GeomTrait, GeomType, ParamConstraint, ParamDefinition, DefaultParamValue, + StatResult, +}; use crate::naming; use crate::plot::types::{DefaultAestheticValue, ParameterValue}; use crate::reader::SqlDialect; @@ -36,38 +39,45 @@ impl GeomTrait for Histogram { } } - fn default_remappings(&self) -> &'static [(&'static str, DefaultAestheticValue)] { - &[ - ("pos1", DefaultAestheticValue::Column("bin")), - ("pos1end", DefaultAestheticValue::Column("bin_end")), - ("pos2", DefaultAestheticValue::Column("count")), - ("pos2end", DefaultAestheticValue::Number(0.0)), - ] + fn default_remappings(&self) -> DefaultAesthetics { + DefaultAesthetics { + defaults: &[ + ("pos1", DefaultAestheticValue::Column("bin")), + ("pos1end", DefaultAestheticValue::Column("bin_end")), + ("pos2", DefaultAestheticValue::Column("count")), + ("pos2end", DefaultAestheticValue::Number(0.0)), + ], + } } fn valid_stat_columns(&self) -> &'static [&'static str] { &["bin", "bin_end", "count", "density"] } - fn default_params(&self) -> &'static [DefaultParam] { - &[ - DefaultParam { + fn default_params(&self) -> &'static [ParamDefinition] { + const PARAMS: &[ParamDefinition] = &[ + ParamDefinition { name: "bins", default: DefaultParamValue::Number(30.0), + constraint: ParamConstraint::count(1.0), }, - DefaultParam { + ParamDefinition { name: "closed", default: DefaultParamValue::String("right"), + constraint: ParamConstraint::string_option(CLOSED_VALUES), }, - DefaultParam { + ParamDefinition { name: "binwidth", default: DefaultParamValue::Null, + constraint: ParamConstraint::number_min_exclusive(0.0), }, - DefaultParam { + ParamDefinition { name: "position", default: DefaultParamValue::String("stack"), + constraint: ParamConstraint::string_option(POSITION_VALUES), }, - ] + ]; + PARAMS } fn stat_consumed_aesthetics(&self) -> &'static [&'static str] { @@ -119,23 +129,17 @@ fn stat_histogram( GgsqlError::ValidationError("Histogram requires 'x' aesthetic mapping".to_string()) })?; - // Get bins from parameters (default: 30) - let bins = parameters - .get("bins") - .and_then(|p| match p { - ParameterValue::Number(n) => Some(*n as usize), - _ => None, - }) - .expect("bins is not the correct format. Expected a number"); + // Get bins from parameters (default: 30, validated by constraint) + let ParameterValue::Number(bins) = parameters.get("bins").unwrap() else { + unreachable!("bins validated by ParamConstraint::count") + }; + let bins = *bins as usize; - // Get closed parameter (default: "right") - let closed = parameters - .get("closed") - .and_then(|p| match p { - ParameterValue::String(s) => Some(s.as_str()), - _ => None, - }) - .expect("closed is not the correct format. Expected a string"); + // Get closed parameter (default: "right", validated by constraint) + let ParameterValue::String(closed) = parameters.get("closed").unwrap() else { + unreachable!("closed validated by ParamConstraint::string_option") + }; + let closed = closed.as_str(); // Get binwidth from parameters (default: None - use bins to calculate) let explicit_binwidth = parameters.get("binwidth").and_then(|p| match p { diff --git a/src/plot/layer/geom/line.rs b/src/plot/layer/geom/line.rs index e480266d..d0a928d0 100644 --- a/src/plot/layer/geom/line.rs +++ b/src/plot/layer/geom/line.rs @@ -1,7 +1,10 @@ //! Line geom implementation -use super::{DefaultAesthetics, DefaultParam, DefaultParamValue, GeomTrait, GeomType, StatResult}; -use crate::plot::layer::orientation::ALIGNED; +use super::{ + DefaultAesthetics, GeomTrait, GeomType, ParamConstraint, ParamDefinition, DefaultParamValue, + StatResult, +}; +use crate::plot::layer::orientation::{ALIGNED, ORIENTATION_VALUES}; use crate::plot::types::DefaultAestheticValue; use crate::{naming, Mappings}; @@ -27,11 +30,13 @@ impl GeomTrait for Line { } } - fn default_params(&self) -> &'static [DefaultParam] { - &[DefaultParam { + fn default_params(&self) -> &'static [ParamDefinition] { + const PARAMS: &[ParamDefinition] = &[ParamDefinition { name: "orientation", default: DefaultParamValue::String(ALIGNED), - }] + constraint: ParamConstraint::string_option(ORIENTATION_VALUES), + }]; + PARAMS } fn needs_stat_transform(&self, _aesthetics: &Mappings) -> bool { diff --git a/src/plot/layer/geom/linear.rs b/src/plot/layer/geom/linear.rs index b56a76b2..1ae4dcb1 100644 --- a/src/plot/layer/geom/linear.rs +++ b/src/plot/layer/geom/linear.rs @@ -1,7 +1,9 @@ //! Linear geom implementation -use super::{DefaultAesthetics, DefaultParam, DefaultParamValue, GeomTrait, GeomType}; -use crate::plot::layer::orientation::ALIGNED; +use super::{ + DefaultAesthetics, GeomTrait, GeomType, ParamConstraint, ParamDefinition, DefaultParamValue, +}; +use crate::plot::layer::orientation::{ALIGNED, ORIENTATION_VALUES}; use crate::plot::types::DefaultAestheticValue; /// Linear geom - lines with coefficient and intercept @@ -26,11 +28,13 @@ impl GeomTrait for Linear { } } - fn default_params(&self) -> &'static [DefaultParam] { - &[DefaultParam { + fn default_params(&self) -> &'static [ParamDefinition] { + const PARAMS: &[ParamDefinition] = &[ParamDefinition { name: "orientation", default: DefaultParamValue::String(ALIGNED), - }] + constraint: ParamConstraint::string_option(ORIENTATION_VALUES), + }]; + PARAMS } } diff --git a/src/plot/layer/geom/mod.rs b/src/plot/layer/geom/mod.rs index ec37038d..79820530 100644 --- a/src/plot/layer/geom/mod.rs +++ b/src/plot/layer/geom/mod.rs @@ -49,7 +49,9 @@ mod text; mod violin; // Re-export types -pub use types::{DefaultAesthetics, DefaultParam, DefaultParamValue, StatResult}; +pub use types::{ + DefaultAesthetics, ParamConstraint, ParamDefinition, DefaultParamValue, StatResult, +}; // Re-export geom structs for direct access if needed pub use area::Area; @@ -72,7 +74,7 @@ pub use smooth::Smooth; pub use text::Text; pub use violin::Violin; -use crate::plot::types::{DefaultAestheticValue, ParameterValue, Schema}; +use crate::plot::types::{ParameterValue, Schema}; use crate::reader::SqlDialect; /// Enum of all geom types for pattern matching and serialization @@ -145,8 +147,8 @@ pub trait GeomTrait: std::fmt::Debug + std::fmt::Display + Send + Sync { /// - `DefaultAestheticValue::Number(0.0)` - maps a literal value to the aesthetic /// /// These defaults can be overridden by a REMAPPING clause. - fn default_remappings(&self) -> &'static [(&'static str, DefaultAestheticValue)] { - &[] + fn default_remappings(&self) -> DefaultAesthetics { + DefaultAesthetics { defaults: &[] } } /// Returns valid stat column names that can be used in REMAPPING (early validation). @@ -167,7 +169,7 @@ pub trait GeomTrait: std::fmt::Debug + std::fmt::Display + Send + Sync { /// Returns non-aesthetic parameters with their default values. /// /// These control stat behavior (e.g., bins for histogram). - fn default_params(&self) -> &'static [DefaultParam] { + fn default_params(&self) -> &'static [ParamDefinition] { &[] } @@ -368,7 +370,7 @@ impl Geom { } /// Get default remappings - pub fn default_remappings(&self) -> &'static [(&'static str, DefaultAestheticValue)] { + pub fn default_remappings(&self) -> DefaultAesthetics { self.0.default_remappings() } @@ -378,7 +380,7 @@ impl Geom { } /// Get default parameters - pub fn default_params(&self) -> &'static [DefaultParam] { + pub fn default_params(&self) -> &'static [ParamDefinition] { self.0.default_params() } @@ -589,7 +591,7 @@ mod tests { aesthetics.defaults.iter().map(|(name, _)| *name).collect(); // Check each remapping name exists in aesthetics - for (name, _) in remappings { + for (name, _) in remappings.defaults { assert!( aesthetic_names.contains(name), "Geom '{}' has '{}' in default_remappings() but not in aesthetics().defaults. \ diff --git a/src/plot/layer/geom/path.rs b/src/plot/layer/geom/path.rs index 39726c29..8adf2152 100644 --- a/src/plot/layer/geom/path.rs +++ b/src/plot/layer/geom/path.rs @@ -1,6 +1,9 @@ //! Path geom implementation -use super::{DefaultAesthetics, DefaultParam, DefaultParamValue, GeomTrait, GeomType}; +use super::types::POSITION_VALUES; +use super::{ + DefaultAesthetics, GeomTrait, GeomType, ParamConstraint, ParamDefinition, DefaultParamValue, +}; use crate::plot::types::DefaultAestheticValue; /// Path geom - connected line segments in order @@ -25,11 +28,13 @@ impl GeomTrait for Path { } } - fn default_params(&self) -> &'static [DefaultParam] { - &[DefaultParam { + fn default_params(&self) -> &'static [ParamDefinition] { + const PARAMS: &[ParamDefinition] = &[ParamDefinition { name: "position", default: DefaultParamValue::String("identity"), - }] + constraint: ParamConstraint::string_option(POSITION_VALUES), + }]; + PARAMS } } diff --git a/src/plot/layer/geom/point.rs b/src/plot/layer/geom/point.rs index 73ce013a..6a2cc33b 100644 --- a/src/plot/layer/geom/point.rs +++ b/src/plot/layer/geom/point.rs @@ -1,6 +1,9 @@ //! Point geom implementation -use super::{DefaultAesthetics, DefaultParam, DefaultParamValue, GeomTrait, GeomType}; +use super::types::POSITION_VALUES; +use super::{ + DefaultAesthetics, GeomTrait, GeomType, ParamConstraint, ParamDefinition, DefaultParamValue, +}; use crate::plot::types::DefaultAestheticValue; /// Point geom - scatter plots and similar @@ -27,11 +30,13 @@ impl GeomTrait for Point { } } - fn default_params(&self) -> &'static [DefaultParam] { - &[DefaultParam { + fn default_params(&self) -> &'static [ParamDefinition] { + const PARAMS: &[ParamDefinition] = &[ParamDefinition { name: "position", default: DefaultParamValue::String("identity"), - }] + constraint: ParamConstraint::string_option(POSITION_VALUES), + }]; + PARAMS } } diff --git a/src/plot/layer/geom/polygon.rs b/src/plot/layer/geom/polygon.rs index 78b9d9b4..a0c60245 100644 --- a/src/plot/layer/geom/polygon.rs +++ b/src/plot/layer/geom/polygon.rs @@ -1,6 +1,9 @@ //! Polygon geom implementation -use super::{DefaultAesthetics, DefaultParam, DefaultParamValue, GeomTrait, GeomType}; +use super::types::POSITION_VALUES; +use super::{ + DefaultAesthetics, GeomTrait, GeomType, ParamConstraint, ParamDefinition, DefaultParamValue, +}; use crate::plot::types::DefaultAestheticValue; /// Polygon geom - arbitrary polygons @@ -26,11 +29,13 @@ impl GeomTrait for Polygon { } } - fn default_params(&self) -> &'static [DefaultParam] { - &[DefaultParam { + fn default_params(&self) -> &'static [ParamDefinition] { + const PARAMS: &[ParamDefinition] = &[ParamDefinition { name: "position", default: DefaultParamValue::String("identity"), - }] + constraint: ParamConstraint::string_option(POSITION_VALUES), + }]; + PARAMS } } diff --git a/src/plot/layer/geom/rect.rs b/src/plot/layer/geom/rect.rs index 8d7a2c90..a03a084e 100644 --- a/src/plot/layer/geom/rect.rs +++ b/src/plot/layer/geom/rect.rs @@ -3,10 +3,11 @@ use std::collections::HashMap; use super::types::get_column_name; -use super::{DefaultAesthetics, GeomTrait, GeomType, StatResult}; +use super::types::POSITION_VALUES; +use super::{DefaultAesthetics, GeomTrait, GeomType, ParamConstraint, StatResult}; use crate::naming; use crate::plot::types::{DefaultAestheticValue, ParameterValue}; -use crate::plot::{DefaultParam, DefaultParamValue}; +use crate::plot::{ParamDefinition, DefaultParamValue}; use crate::{DataFrame, GgsqlError, Mappings, Result}; use super::types::Schema; @@ -50,27 +51,31 @@ impl GeomTrait for Rect { } } - fn default_remappings(&self) -> &'static [(&'static str, DefaultAestheticValue)] { - &[ - // For continuous scales: remap to min/max - ("pos1min", DefaultAestheticValue::Column("pos1min")), - ("pos1max", DefaultAestheticValue::Column("pos1max")), - ("pos2min", DefaultAestheticValue::Column("pos2min")), - ("pos2max", DefaultAestheticValue::Column("pos2max")), - // For discrete scales: remap to center - ("pos1", DefaultAestheticValue::Column("pos1")), - ("pos2", DefaultAestheticValue::Column("pos2")), - // Width/height passed through for discrete (writer validation) - ("width", DefaultAestheticValue::Column("width")), - ("height", DefaultAestheticValue::Column("height")), - ] + fn default_remappings(&self) -> DefaultAesthetics { + DefaultAesthetics { + defaults: &[ + // For continuous scales: remap to min/max + ("pos1min", DefaultAestheticValue::Column("pos1min")), + ("pos1max", DefaultAestheticValue::Column("pos1max")), + ("pos2min", DefaultAestheticValue::Column("pos2min")), + ("pos2max", DefaultAestheticValue::Column("pos2max")), + // For discrete scales: remap to center + ("pos1", DefaultAestheticValue::Column("pos1")), + ("pos2", DefaultAestheticValue::Column("pos2")), + // Width/height passed through for discrete (writer validation) + ("width", DefaultAestheticValue::Column("width")), + ("height", DefaultAestheticValue::Column("height")), + ], + } } - fn default_params(&self) -> &'static [DefaultParam] { - &[DefaultParam { + fn default_params(&self) -> &'static [ParamDefinition] { + const PARAMS: &[ParamDefinition] = &[ParamDefinition { name: "position", default: DefaultParamValue::String("identity"), - }] + constraint: ParamConstraint::string_option(POSITION_VALUES), + }]; + PARAMS } fn valid_stat_columns(&self) -> &'static [&'static str] { diff --git a/src/plot/layer/geom/ribbon.rs b/src/plot/layer/geom/ribbon.rs index 6d57752a..e72dbf95 100644 --- a/src/plot/layer/geom/ribbon.rs +++ b/src/plot/layer/geom/ribbon.rs @@ -1,8 +1,9 @@ //! Ribbon geom implementation +use super::types::POSITION_VALUES; use super::{DefaultAesthetics, GeomTrait, GeomType, StatResult}; use crate::plot::types::DefaultAestheticValue; -use crate::plot::{DefaultParam, DefaultParamValue}; +use crate::plot::{ParamConstraint, ParamDefinition, DefaultParamValue}; use crate::{naming, Mappings}; /// Ribbon geom - confidence bands and ranges @@ -29,11 +30,13 @@ impl GeomTrait for Ribbon { } } - fn default_params(&self) -> &'static [DefaultParam] { - &[DefaultParam { + fn default_params(&self) -> &'static [ParamDefinition] { + const PARAMS: &[ParamDefinition] = &[ParamDefinition { name: "position", default: DefaultParamValue::String("identity"), - }] + constraint: ParamConstraint::string_option(POSITION_VALUES), + }]; + PARAMS } fn needs_stat_transform(&self, _aesthetics: &Mappings) -> bool { diff --git a/src/plot/layer/geom/segment.rs b/src/plot/layer/geom/segment.rs index 3126f352..94d68a90 100644 --- a/src/plot/layer/geom/segment.rs +++ b/src/plot/layer/geom/segment.rs @@ -1,6 +1,9 @@ //! Segment geom implementation -use super::{DefaultAesthetics, DefaultParam, DefaultParamValue, GeomTrait, GeomType}; +use super::types::POSITION_VALUES; +use super::{ + DefaultAesthetics, GeomTrait, GeomType, ParamConstraint, ParamDefinition, DefaultParamValue, +}; use crate::plot::types::DefaultAestheticValue; /// Segment geom - line segments between two points @@ -27,11 +30,13 @@ impl GeomTrait for Segment { } } - fn default_params(&self) -> &'static [DefaultParam] { - &[DefaultParam { + fn default_params(&self) -> &'static [ParamDefinition] { + const PARAMS: &[ParamDefinition] = &[ParamDefinition { name: "position", default: DefaultParamValue::String("identity"), - }] + constraint: ParamConstraint::string_option(POSITION_VALUES), + }]; + PARAMS } } diff --git a/src/plot/layer/geom/smooth.rs b/src/plot/layer/geom/smooth.rs index 456f293e..f6bcf664 100644 --- a/src/plot/layer/geom/smooth.rs +++ b/src/plot/layer/geom/smooth.rs @@ -1,12 +1,29 @@ //! Smooth geom implementation -use super::{DefaultAesthetics, DefaultParam, DefaultParamValue, GeomTrait, GeomType}; +use super::types::POSITION_VALUES; +use super::{ + DefaultAesthetics, GeomTrait, GeomType, ParamConstraint, ParamDefinition, DefaultParamValue, +}; use crate::plot::geom::types::get_column_name; use crate::plot::types::DefaultAestheticValue; use crate::plot::{ParameterValue, StatResult}; use crate::reader::SqlDialect; use crate::{naming, GgsqlError, Mappings, Result}; +/// Valid methods for smoothing +const METHOD_VALUES: &[&str] = &["nw", "nadaraya-watson", "ols", "tls"]; +/// Valid kernel types for smooth density estimation +const KERNEL_VALUES: &[&str] = &[ + "gaussian", + "epanechnikov", + "triangular", + "rectangular", + "uniform", + "biweight", + "quartic", + "cosine", +]; + /// Smooth geom - smoothed conditional means (regression, LOESS, etc.) #[derive(Debug, Clone, Copy)] pub struct Smooth; @@ -30,40 +47,48 @@ impl GeomTrait for Smooth { } } - fn default_params(&self) -> &'static [DefaultParam] { - &[ - DefaultParam { + fn default_params(&self) -> &'static [ParamDefinition] { + const PARAMS: &[ParamDefinition] = &[ + ParamDefinition { name: "position", default: DefaultParamValue::String("identity"), + constraint: ParamConstraint::string_option(POSITION_VALUES), }, - DefaultParam { + ParamDefinition { name: "method", default: DefaultParamValue::String("nw"), + constraint: ParamConstraint::string_option(METHOD_VALUES), }, - DefaultParam { + ParamDefinition { name: "bandwidth", default: DefaultParamValue::Null, + constraint: ParamConstraint::number_min_exclusive(0.0), }, - DefaultParam { + ParamDefinition { name: "adjust", default: DefaultParamValue::Number(1.0), + constraint: ParamConstraint::number_min_exclusive(0.0), }, - DefaultParam { + ParamDefinition { name: "kernel", default: DefaultParamValue::String("gaussian"), + constraint: ParamConstraint::string_option(KERNEL_VALUES), }, - ] + ]; + PARAMS } fn needs_stat_transform(&self, _aesthetics: &Mappings) -> bool { true } - fn default_remappings(&self) -> &'static [(&'static str, DefaultAestheticValue)] { - &[ - ("pos1", DefaultAestheticValue::Column("pos1")), - ("pos2", DefaultAestheticValue::Column("intensity")), - ] + fn default_remappings(&self) -> DefaultAesthetics { + DefaultAesthetics { + defaults: &[ + ("pos1", DefaultAestheticValue::Column("pos1")), + ("pos2", DefaultAestheticValue::Column("intensity")), + ], + } } fn apply_stat_transform( @@ -76,10 +101,9 @@ impl GeomTrait for Smooth { _execute_query: &dyn Fn(&str) -> crate::Result, dialect: &dyn SqlDialect, ) -> crate::Result { - let Some(ParameterValue::String(method)) = parameters.get("method") else { - return Err(GgsqlError::ValidationError( - "The `method` setting must be a string.".to_string(), - )); + // Get method from parameters (validated by ParamConstraint::string_option) + let ParameterValue::String(method) = parameters.get("method").unwrap() else { + unreachable!("method validated by ParamConstraint::string_option") }; match method.as_str() { @@ -100,9 +124,7 @@ impl GeomTrait for Smooth { } "ols" => stat_ols(query, aesthetics, group_by), "tls" => stat_tls(query, aesthetics, group_by), - _ => Err(GgsqlError::ValidationError( - "The `method` setting must be 'nw', 'ols', or 'tls'.".to_string(), - )), + _ => unreachable!("method validated by ParamConstraint::string_option"), } } } diff --git a/src/plot/layer/geom/text.rs b/src/plot/layer/geom/text.rs index f7bfcb46..cc0a8e84 100644 --- a/src/plot/layer/geom/text.rs +++ b/src/plot/layer/geom/text.rs @@ -1,8 +1,12 @@ //! Text geom implementation -use super::{DefaultAesthetics, GeomTrait, GeomType}; +use super::types::POSITION_VALUES; +use super::{ + DefaultAesthetics, GeomTrait, GeomType, ParamConstraint, ParamDefinition, DefaultParamValue, + ParameterValue, +}; use crate::plot::types::DefaultAestheticValue; -use crate::plot::{DefaultParam, DefaultParamValue, ParameterValue}; +use crate::plot::{ArrayConstraint, NumberConstraint}; use crate::{naming, DataFrame, Result}; use std::collections::HashMap; @@ -35,21 +39,28 @@ impl GeomTrait for Text { } } - fn default_params(&self) -> &'static [DefaultParam] { - &[ - DefaultParam { + fn default_params(&self) -> &'static [ParamDefinition] { + const PARAMS: &[ParamDefinition] = &[ + ParamDefinition { + name: "position", + default: DefaultParamValue::String("identity"), + constraint: ParamConstraint::string_option(POSITION_VALUES), + }, + ParamDefinition { name: "offset", default: DefaultParamValue::Null, + constraint: ParamConstraint::number_or_numeric_array( + NumberConstraint::unconstrained(), + ArrayConstraint::of_numbers_len(NumberConstraint::unconstrained(), 2), + ), }, - DefaultParam { + ParamDefinition { name: "format", default: DefaultParamValue::Null, + constraint: ParamConstraint::string(), }, - DefaultParam { - name: "position", - default: DefaultParamValue::String("identity"), - }, - ] + ]; + PARAMS } fn post_process( diff --git a/src/plot/layer/geom/types.rs b/src/plot/layer/geom/types.rs index 5e04f386..21dddb5f 100644 --- a/src/plot/layer/geom/types.rs +++ b/src/plot/layer/geom/types.rs @@ -6,7 +6,17 @@ use crate::plot::aesthetic::parse_positional; use crate::{plot::types::DefaultAestheticValue, Mappings}; // Re-export shared types from the central location -pub use crate::plot::types::{DefaultParam, DefaultParamValue}; +pub use crate::plot::types::{ParamConstraint, ParamDefinition, DefaultParamValue}; + +// ============================================================================= +// Common constraint value arrays +// ============================================================================= + +/// Standard position adjustment values for the `position` parameter +pub const POSITION_VALUES: &[&str] = &["identity", "stack", "dodge", "jitter"]; + +/// Closed interval side values for binned data +pub const CLOSED_VALUES: &[&str] = &["left", "right"]; /// Default aesthetic values for a geom type /// diff --git a/src/plot/layer/geom/violin.rs b/src/plot/layer/geom/violin.rs index b213ebb7..c723f9f4 100644 --- a/src/plot/layer/geom/violin.rs +++ b/src/plot/layer/geom/violin.rs @@ -1,17 +1,30 @@ //! Violin geom implementation +use super::types::POSITION_VALUES; use super::{DefaultAesthetics, GeomTrait, GeomType, StatResult}; use crate::{ naming, plot::{ - geom::types::get_column_name, DefaultAestheticValue, DefaultParam, DefaultParamValue, - ParameterValue, + geom::types::get_column_name, DefaultAestheticValue, ParamConstraint, ParamDefinition, + DefaultParamValue, ParameterValue, }, DataFrame, GgsqlError, Mappings, Result, }; use polars::prelude::*; use std::collections::HashMap; +/// Valid kernel types for violin density estimation +const KERNEL_VALUES: &[&str] = &[ + "gaussian", + "epanechnikov", + "triangular", + "rectangular", + "uniform", + "biweight", + "quartic", + "cosine", +]; + /// Violin geom - violin plots (mirrored density) #[derive(Debug, Clone, Copy)] pub struct Violin; @@ -41,40 +54,49 @@ impl GeomTrait for Violin { true } - fn default_params(&self) -> &'static [DefaultParam] { - &[ - DefaultParam { + fn default_params(&self) -> &'static [ParamDefinition] { + const PARAMS: &[ParamDefinition] = &[ + ParamDefinition { name: "bandwidth", default: DefaultParamValue::Null, + constraint: ParamConstraint::number_min_exclusive(0.0), }, - DefaultParam { + ParamDefinition { name: "adjust", default: DefaultParamValue::Number(1.0), + constraint: ParamConstraint::number_min_exclusive(0.0), }, - DefaultParam { + ParamDefinition { name: "kernel", default: DefaultParamValue::String("gaussian"), + constraint: ParamConstraint::string_option(KERNEL_VALUES), }, - DefaultParam { + ParamDefinition { name: "position", default: DefaultParamValue::String("dodge"), + constraint: ParamConstraint::string_option(POSITION_VALUES), }, - DefaultParam { + ParamDefinition { name: "width", default: DefaultParamValue::Number(0.9), + constraint: ParamConstraint::number_range(0.0, 1.0), }, - DefaultParam { + ParamDefinition { name: "tails", default: DefaultParamValue::Number(3.0), + constraint: ParamConstraint::number_min(0.0), }, - ] + ]; + PARAMS } - fn default_remappings(&self) -> &'static [(&'static str, DefaultAestheticValue)] { - &[ - ("pos2", DefaultAestheticValue::Column("pos2")), - ("offset", DefaultAestheticValue::Column("density")), - ] + fn default_remappings(&self) -> DefaultAesthetics { + DefaultAesthetics { + defaults: &[ + ("pos2", DefaultAestheticValue::Column("pos2")), + ("offset", DefaultAestheticValue::Column("density")), + ], + } } fn valid_stat_columns(&self) -> &'static [&'static str] { diff --git a/src/plot/layer/mod.rs b/src/plot/layer/mod.rs index d539e195..5cf34a5e 100644 --- a/src/plot/layer/mod.rs +++ b/src/plot/layer/mod.rs @@ -20,7 +20,7 @@ pub use orientation::is_transposed; // Re-export geom types for convenience pub use geom::{ - DefaultAesthetics, DefaultParam, DefaultParamValue, Geom, GeomTrait, GeomType, StatResult, + DefaultAesthetics, Geom, GeomTrait, GeomType, ParamDefinition, DefaultParamValue, StatResult, }; // Re-export position types for convenience @@ -29,7 +29,9 @@ pub use position::{Position, PositionTrait, PositionType}; use crate::{ plot::{ is_facet_aesthetic, parse_positional, - types::{AestheticValue, DataSource, Mappings, ParameterValue, SqlExpression}, + types::{ + validate_parameter, AestheticValue, DataSource, Mappings, ParameterValue, SqlExpression, + }, }, AestheticContext, }; @@ -383,21 +385,42 @@ impl Layer { /// Validate that all SETTING parameters are valid for this layer's geom and position pub fn validate_settings(&self) -> std::result::Result<(), String> { - // Combine valid settings from both geom and position + // Combine valid settings from both geom and position (includes aesthetics) let mut valid = self.geom.valid_settings(); valid.extend(self.position.valid_settings()); - for param_name in self.parameters.keys() { + for (param_name, value) in self.parameters.iter() { + // Check if this is a valid setting at all if !valid.contains(¶m_name.as_str()) { return Err(format!( - "Invalid setting '{}' for geom '{}' with position '{}'. Valid settings are: {}", - param_name, + "{} layer setting should be {}, not '{}'", self.geom, - self.position, - valid.join(", ") + crate::or_list_quoted(&valid, '\''), + param_name )); } + + // Validate against constraints if this is a geom param + if let Some(param) = self + .geom + .default_params() + .iter() + .find(|p| p.name == param_name) + { + validate_parameter(param_name, value, ¶m.constraint)?; + } + // Or a position param + else if let Some(param) = self + .position + .default_params() + .iter() + .find(|p| p.name == param_name) + { + validate_parameter(param_name, value, ¶m.constraint)?; + } + // Otherwise it's a valid aesthetic setting (no constraint validation needed) } + Ok(()) } diff --git a/src/plot/layer/orientation.rs b/src/plot/layer/orientation.rs index 28f064cb..b77caae8 100644 --- a/src/plot/layer/orientation.rs +++ b/src/plot/layer/orientation.rs @@ -36,6 +36,9 @@ pub const ALIGNED: &str = "aligned"; /// Orientation value for transposed/horizontal orientation. pub const TRANSPOSED: &str = "transposed"; +/// Valid orientation values for constraint validation. +pub const ORIENTATION_VALUES: &[&str] = &[ALIGNED, TRANSPOSED]; + /// Determine effective orientation for a layer. /// /// Returns explicit orientation if set via SETTING, otherwise auto-detects diff --git a/src/plot/layer/position/dodge.rs b/src/plot/layer/position/dodge.rs index fa1b43fb..3bcde282 100644 --- a/src/plot/layer/position/dodge.rs +++ b/src/plot/layer/position/dodge.rs @@ -7,7 +7,7 @@ //! - If both are discrete → 2D grid dodge (both offsets, arranged in a grid) use super::{compute_dodge_offsets, is_continuous_scale, Layer, PositionTrait, PositionType}; -use crate::plot::types::{DefaultParam, DefaultParamValue, ParameterValue}; +use crate::plot::types::{ParamConstraint, ParamDefinition, DefaultParamValue, ParameterValue}; use crate::{naming, DataFrame, GgsqlError, Plot, Result}; use polars::prelude::*; use std::collections::HashMap; @@ -95,11 +95,13 @@ impl PositionTrait for Dodge { PositionType::Dodge } - fn default_params(&self) -> &'static [DefaultParam] { - &[DefaultParam { + fn default_params(&self) -> &'static [ParamDefinition] { + const PARAMS: &[ParamDefinition] = &[ParamDefinition { name: "width", default: DefaultParamValue::Number(0.9), - }] + constraint: ParamConstraint::number_range(0.0, 1.0), + }]; + PARAMS } fn creates_pos1offset(&self) -> bool { @@ -634,6 +636,9 @@ mod tests { let params = dodge.default_params(); assert_eq!(params.len(), 1); assert_eq!(params[0].name, "width"); - assert!(matches!(params[0].default, DefaultParamValue::Number(0.9))); + assert!(matches!( + params[0].default, + DefaultParamValue::Number(0.9) + )); } } diff --git a/src/plot/layer/position/jitter.rs b/src/plot/layer/position/jitter.rs index c5cd509e..69e66a5d 100644 --- a/src/plot/layer/position/jitter.rs +++ b/src/plot/layer/position/jitter.rs @@ -18,11 +18,14 @@ use super::{ compute_dodge_offsets, compute_group_indices, is_continuous_scale, Layer, PositionTrait, PositionType, }; -use crate::plot::types::{DefaultParam, DefaultParamValue, ParameterValue}; +use crate::plot::types::{ParamConstraint, ParamDefinition, DefaultParamValue, ParameterValue}; use crate::{naming, DataFrame, GgsqlError, Plot, Result}; use polars::prelude::*; use rand::Rng; +/// Valid distribution types for jitter position +const DISTRIBUTION_VALUES: &[&str] = &["uniform", "normal", "density", "intensity"]; + /// Jitter distribution type #[derive(Debug, Clone, Copy, PartialEq, Eq)] enum JitterDistribution { @@ -265,30 +268,36 @@ impl PositionTrait for Jitter { PositionType::Jitter } - fn default_params(&self) -> &'static [DefaultParam] { - &[ - DefaultParam { + fn default_params(&self) -> &'static [ParamDefinition] { + const PARAMS: &[ParamDefinition] = &[ + ParamDefinition { name: "width", default: DefaultParamValue::Number(0.9), + constraint: ParamConstraint::number_range(0.0, 1.0), }, - DefaultParam { + ParamDefinition { name: "dodge", default: DefaultParamValue::Boolean(true), + constraint: ParamConstraint::boolean(), }, - DefaultParam { + ParamDefinition { name: "distribution", default: DefaultParamValue::String("uniform"), + constraint: ParamConstraint::string_option(DISTRIBUTION_VALUES), }, // Density distribution parameters (match violin/density geoms) - DefaultParam { + ParamDefinition { name: "bandwidth", default: DefaultParamValue::Null, + constraint: ParamConstraint::number_min_exclusive(0.0), }, - DefaultParam { + ParamDefinition { name: "adjust", default: DefaultParamValue::Number(1.0), + constraint: ParamConstraint::number_min_exclusive(0.0), }, - ] + ]; + PARAMS } fn creates_pos1offset(&self) -> bool { @@ -987,7 +996,10 @@ mod tests { let params = jitter.default_params(); assert_eq!(params.len(), 5); assert_eq!(params[0].name, "width"); - assert!(matches!(params[0].default, DefaultParamValue::Number(0.9))); + assert!(matches!( + params[0].default, + DefaultParamValue::Number(0.9) + )); assert_eq!(params[1].name, "dodge"); assert!(matches!( params[1].default, @@ -1002,7 +1014,10 @@ mod tests { assert_eq!(params[3].name, "bandwidth"); assert!(matches!(params[3].default, DefaultParamValue::Null)); assert_eq!(params[4].name, "adjust"); - assert!(matches!(params[4].default, DefaultParamValue::Number(1.0))); + assert!(matches!( + params[4].default, + DefaultParamValue::Number(1.0) + )); } #[test] @@ -1034,9 +1049,11 @@ mod tests { // Normal distribution is centered at 0 // Values can exceed the width bounds (unlike uniform), but should be centered + // With only 4 samples and σ = width/4 = 0.225, the standard error is ~0.11 + // Use a wide tolerance to avoid flaky tests with small sample sizes let mean: f64 = offsets.iter().sum::() / offsets.len() as f64; assert!( - mean.abs() < 0.3, // Should be roughly centered (with 4 values, some variance expected) + mean.abs() < 0.5, "Normal distribution mean {} should be close to 0", mean ); diff --git a/src/plot/layer/position/mod.rs b/src/plot/layer/position/mod.rs index cdd0945a..694a5d7f 100644 --- a/src/plot/layer/position/mod.rs +++ b/src/plot/layer/position/mod.rs @@ -14,7 +14,7 @@ mod identity; mod jitter; mod stack; -use crate::plot::types::{DefaultParam, DefaultParamValue, ParameterValue}; +use crate::plot::types::{ParamDefinition, DefaultParamValue, ParameterValue}; use crate::plot::ScaleTypeKind; use crate::{DataFrame, Plot, Result}; use serde::{Deserialize, Serialize}; @@ -143,7 +143,7 @@ pub trait PositionTrait: std::fmt::Debug + std::fmt::Display + Send + Sync { fn position_type(&self) -> PositionType; /// Returns default parameter values for this position - fn default_params(&self) -> &'static [DefaultParam] { + fn default_params(&self) -> &'static [ParamDefinition] { &[] } @@ -224,7 +224,7 @@ impl Position { } /// Get default parameters - pub fn default_params(&self) -> &'static [DefaultParam] { + pub fn default_params(&self) -> &'static [ParamDefinition] { self.0.default_params() } diff --git a/src/plot/layer/position/stack.rs b/src/plot/layer/position/stack.rs index 02eb1f3c..f669f18a 100644 --- a/src/plot/layer/position/stack.rs +++ b/src/plot/layer/position/stack.rs @@ -7,7 +7,7 @@ //! - If pos1 is continuous and pos2 is discrete → stack horizontally (modify pos1/pos1end) use super::{is_continuous_scale, Layer, PositionTrait, PositionType}; -use crate::plot::types::{DefaultParam, DefaultParamValue, ParameterValue}; +use crate::plot::types::{ParamConstraint, ParamDefinition, DefaultParamValue, ParameterValue}; use crate::{naming, DataFrame, GgsqlError, Plot, Result}; use polars::prelude::*; @@ -37,17 +37,20 @@ impl PositionTrait for Stack { PositionType::Stack } - fn default_params(&self) -> &'static [DefaultParam] { - &[ - DefaultParam { + fn default_params(&self) -> &'static [ParamDefinition] { + const PARAMS: &[ParamDefinition] = &[ + ParamDefinition { name: "center", default: DefaultParamValue::Boolean(false), + constraint: ParamConstraint::boolean(), }, - DefaultParam { + ParamDefinition { name: "total", default: DefaultParamValue::Null, + constraint: ParamConstraint::number_min_exclusive(0.0), }, - ] + ]; + PARAMS } fn apply_adjustment( diff --git a/src/plot/main.rs b/src/plot/main.rs index e014d400..d37a418b 100644 --- a/src/plot/main.rs +++ b/src/plot/main.rs @@ -32,7 +32,7 @@ pub use super::types::{ // Re-export Geom and related types from the layer::geom module pub use super::layer::geom::{ - DefaultAesthetics, DefaultParam, DefaultParamValue, Geom, GeomTrait, GeomType, StatResult, + DefaultAesthetics, Geom, GeomTrait, GeomType, ParamDefinition, DefaultParamValue, StatResult, }; // Re-export Layer from the layer module diff --git a/src/plot/projection/coord/cartesian.rs b/src/plot/projection/coord/cartesian.rs index 425026c0..4135febe 100644 --- a/src/plot/projection/coord/cartesian.rs +++ b/src/plot/projection/coord/cartesian.rs @@ -1,7 +1,7 @@ //! Cartesian coordinate system implementation use super::{CoordKind, CoordTrait}; -use crate::plot::types::{DefaultParam, DefaultParamValue}; +use crate::plot::types::{ParamConstraint, ParamDefinition, DefaultParamValue}; /// Cartesian coordinate system - standard x/y coordinates #[derive(Debug, Clone, Copy)] @@ -20,17 +20,20 @@ impl CoordTrait for Cartesian { &["x", "y"] } - fn default_properties(&self) -> &'static [DefaultParam] { - &[ - DefaultParam { + fn default_properties(&self) -> &'static [ParamDefinition] { + const PARAMS: &[ParamDefinition] = &[ + ParamDefinition { name: "ratio", default: DefaultParamValue::Null, + constraint: ParamConstraint::number_min_exclusive(0.0), }, - DefaultParam { + ParamDefinition { name: "clip", - default: DefaultParamValue::Null, + default: DefaultParamValue::Boolean(true), + constraint: ParamConstraint::boolean(), }, - ] + ]; + PARAMS } } @@ -83,7 +86,6 @@ mod tests { let resolved = cartesian.resolve_properties(&props); assert!(resolved.is_err()); let err = resolved.unwrap_err(); - assert!(err.contains("unknown")); - assert!(err.contains("not valid")); + assert!(err.contains("not 'unknown'")); } } diff --git a/src/plot/projection/coord/mod.rs b/src/plot/projection/coord/mod.rs index 825e6584..25d7d845 100644 --- a/src/plot/projection/coord/mod.rs +++ b/src/plot/projection/coord/mod.rs @@ -24,7 +24,7 @@ use serde::{Deserialize, Serialize}; use std::collections::HashMap; use std::sync::Arc; -use crate::plot::types::DefaultParam; +use crate::plot::types::{validate_parameter, ParamDefinition}; use crate::plot::ParameterValue; // Coord type implementations @@ -75,7 +75,7 @@ pub trait CoordTrait: std::fmt::Debug + std::fmt::Display + Send + Sync { /// Returns list of allowed properties with their default values. /// Default: empty (no properties allowed). - fn default_properties(&self) -> &'static [DefaultParam] { + fn default_properties(&self) -> &'static [ParamDefinition] { &[] } @@ -86,22 +86,27 @@ pub trait CoordTrait: std::fmt::Debug + std::fmt::Display + Send + Sync { properties: &HashMap, ) -> Result, String> { let defaults = self.default_properties(); - let allowed: Vec<&str> = defaults.iter().map(|p| p.name).collect(); - // Check for unknown properties - for key in properties.keys() { - if !allowed.contains(&key.as_str()) { - let valid_props = if allowed.is_empty() { - "none".to_string() + // Validate values against constraints + for (key, value) in properties.iter() { + if let Some(param) = defaults.iter().find(|p| p.name == key) { + validate_parameter(key, value, ¶m.constraint)?; + } else { + let allowed: Vec<&str> = defaults.iter().map(|p| p.name).collect(); + return Err(if allowed.is_empty() { + format!( + "{} projection does not accept any properties, but got '{}'", + self.name(), + key + ) } else { - allowed.join(", ") - }; - return Err(format!( - "Property '{}' not valid for {} projection. Valid properties: {}", - key, - self.name(), - valid_props - )); + format!( + "{} projection property should be {}, not '{}'", + self.name(), + crate::or_list_quoted(&allowed, '\''), + key + ) + }); } } @@ -166,7 +171,7 @@ impl Coord { } /// Returns list of allowed properties with their default values. - pub fn default_properties(&self) -> &'static [DefaultParam] { + pub fn default_properties(&self) -> &'static [ParamDefinition] { self.0.default_properties() } diff --git a/src/plot/projection/coord/polar.rs b/src/plot/projection/coord/polar.rs index b881b018..94902890 100644 --- a/src/plot/projection/coord/polar.rs +++ b/src/plot/projection/coord/polar.rs @@ -1,7 +1,7 @@ //! Polar coordinate system implementation use super::{CoordKind, CoordTrait}; -use crate::plot::types::{DefaultParam, DefaultParamValue}; +use crate::plot::types::{ParamConstraint, ParamDefinition, DefaultParamValue}; /// Polar coordinate system - for pie charts, rose plots #[derive(Debug, Clone, Copy)] @@ -20,25 +20,30 @@ impl CoordTrait for Polar { &["radius", "angle"] } - fn default_properties(&self) -> &'static [DefaultParam] { - &[ - DefaultParam { + fn default_properties(&self) -> &'static [ParamDefinition] { + const PARAMS: &[ParamDefinition] = &[ + ParamDefinition { name: "clip", - default: DefaultParamValue::Null, + default: DefaultParamValue::Boolean(true), + constraint: ParamConstraint::boolean(), }, - DefaultParam { + ParamDefinition { name: "start", default: DefaultParamValue::Number(0.0), // 0 degrees = 12 o'clock + constraint: ParamConstraint::number_range(-360.0, 360.0), }, - DefaultParam { + ParamDefinition { name: "end", default: DefaultParamValue::Null, + constraint: ParamConstraint::number_range(-360.0, 360.0), }, - DefaultParam { + ParamDefinition { name: "inner", default: DefaultParamValue::Null, + constraint: ParamConstraint::number_range(0.0, 1.0), }, - ] + ]; + PARAMS } } @@ -96,8 +101,7 @@ mod tests { let resolved = polar.resolve_properties(&props); assert!(resolved.is_err()); let err = resolved.unwrap_err(); - assert!(err.contains("unknown")); - assert!(err.contains("not valid")); + assert!(err.contains("not 'unknown'")); } #[test] diff --git a/src/plot/scale/scale_type/binned.rs b/src/plot/scale/scale_type/binned.rs index b4bf9d55..3618de3f 100644 --- a/src/plot/scale/scale_type/binned.rs +++ b/src/plot/scale/scale_type/binned.rs @@ -6,9 +6,11 @@ use polars::prelude::DataType; use super::{ expand_numeric_range, resolve_common_steps, ScaleDataContext, ScaleTypeKind, ScaleTypeTrait, - TransformKind, OOB_CENSOR, OOB_SQUISH, + TransformKind, CLOSED_VALUES, OOB_CENSOR, OOB_SQUISH, OOB_VALUES_BINNED, +}; +use crate::plot::types::{ + ArrayConstraint, NumberConstraint, ParamConstraint, ParamDefinition, DefaultParamValue, }; -use crate::plot::types::{DefaultParam, DefaultParamValue}; use crate::plot::{ArrayElement, ParameterValue}; use super::InputRange; @@ -146,37 +148,52 @@ impl ScaleTypeTrait for Binned { TransformKind::Identity } - fn default_properties(&self) -> &'static [DefaultParam] { - &[ - DefaultParam { + fn default_properties(&self) -> &'static [ParamDefinition] { + const PARAMS: &[ParamDefinition] = &[ + ParamDefinition { name: "expand", default: DefaultParamValue::Number(super::DEFAULT_EXPAND_MULT), + // Number (multiplier >= 0) or Array of exactly 2 numbers [mult, add] (both >= 0) + constraint: ParamConstraint::number_or_numeric_array( + NumberConstraint::min(0.0), + ArrayConstraint::of_numbers_len(NumberConstraint::min(0.0), 2), + ), }, - // Binned scales always use "censor" - "keep" is not valid for binned - DefaultParam { + // Binned scales support "censor" and "squish", but not "keep" + ParamDefinition { name: "oob", default: DefaultParamValue::String(OOB_CENSOR), + constraint: ParamConstraint::string_option(OOB_VALUES_BINNED), }, - DefaultParam { + ParamDefinition { name: "reverse", default: DefaultParamValue::Boolean(false), + constraint: ParamConstraint::boolean(), }, - DefaultParam { + ParamDefinition { name: "breaks", default: DefaultParamValue::Number( super::super::breaks::DEFAULT_BREAK_COUNT as f64, ), + // Number (count >= 1), Array of numbers (explicit breaks), or String (temporal interval) + constraint: ParamConstraint::number_or_array_or_string( + NumberConstraint::min(1.0), + ArrayConstraint::of_numbers(NumberConstraint::unconstrained()), + ), }, - DefaultParam { + ParamDefinition { name: "pretty", default: DefaultParamValue::Boolean(true), + constraint: ParamConstraint::boolean(), }, // "left" means bins are [lower, upper), "right" means (lower, upper] - DefaultParam { + ParamDefinition { name: "closed", default: DefaultParamValue::String("left"), + constraint: ParamConstraint::string_option(CLOSED_VALUES), }, - ] + ]; + PARAMS } fn default_output_range( diff --git a/src/plot/scale/scale_type/continuous.rs b/src/plot/scale/scale_type/continuous.rs index e769636d..e5ed9d20 100644 --- a/src/plot/scale/scale_type/continuous.rs +++ b/src/plot/scale/scale_type/continuous.rs @@ -2,8 +2,12 @@ use polars::prelude::DataType; -use super::{ScaleTypeKind, ScaleTypeTrait, TransformKind, OOB_CENSOR, OOB_SQUISH}; -use crate::plot::types::{DefaultParam, DefaultParamValue}; +use super::{ + ScaleTypeKind, ScaleTypeTrait, TransformKind, OOB_CENSOR, OOB_SQUISH, OOB_VALUES_CONTINUOUS, +}; +use crate::plot::types::{ + ArrayConstraint, NumberConstraint, ParamConstraint, ParamDefinition, DefaultParamValue, +}; use crate::plot::{ArrayElement, ParameterValue}; /// Continuous scale type - for continuous numeric data @@ -91,31 +95,45 @@ impl ScaleTypeTrait for Continuous { TransformKind::Identity } - fn default_properties(&self) -> &'static [DefaultParam] { - &[ - DefaultParam { + fn default_properties(&self) -> &'static [ParamDefinition] { + const PARAMS: &[ParamDefinition] = &[ + ParamDefinition { name: "expand", default: DefaultParamValue::Number(super::DEFAULT_EXPAND_MULT), + // Number (multiplier >= 0) or Array of exactly 2 numbers [mult, add] (both >= 0) + constraint: ParamConstraint::number_or_numeric_array( + NumberConstraint::min(0.0), + ArrayConstraint::of_numbers_len(NumberConstraint::min(0.0), 2), + ), }, - DefaultParam { + ParamDefinition { name: "oob", default: DefaultParamValue::Null, // varies by aesthetic + constraint: ParamConstraint::string_option(OOB_VALUES_CONTINUOUS), }, - DefaultParam { + ParamDefinition { name: "reverse", default: DefaultParamValue::Boolean(false), + constraint: ParamConstraint::boolean(), }, - DefaultParam { + ParamDefinition { name: "breaks", default: DefaultParamValue::Number( super::super::breaks::DEFAULT_BREAK_COUNT as f64, ), + // Number (count >= 1), Array of numbers (explicit breaks), or String (temporal interval) + constraint: ParamConstraint::number_or_array_or_string( + NumberConstraint::min(1.0), + ArrayConstraint::of_numbers(NumberConstraint::unconstrained()), + ), }, - DefaultParam { + ParamDefinition { name: "pretty", default: DefaultParamValue::Boolean(true), + constraint: ParamConstraint::boolean(), }, - ] + ]; + PARAMS } fn default_output_range( diff --git a/src/plot/scale/scale_type/discrete.rs b/src/plot/scale/scale_type/discrete.rs index a81686ce..ca0a36c1 100644 --- a/src/plot/scale/scale_type/discrete.rs +++ b/src/plot/scale/scale_type/discrete.rs @@ -4,7 +4,7 @@ use polars::prelude::DataType; use super::super::transform::{Transform, TransformKind}; use super::{ScaleTypeKind, ScaleTypeTrait}; -use crate::plot::types::{DefaultParam, DefaultParamValue}; +use crate::plot::types::{ParamConstraint, ParamDefinition, DefaultParamValue}; use crate::plot::ArrayElement; /// Discrete scale type - for categorical/discrete data @@ -56,12 +56,14 @@ impl ScaleTypeTrait for Discrete { true } - fn default_properties(&self) -> &'static [DefaultParam] { + fn default_properties(&self) -> &'static [ParamDefinition] { // Discrete scales always censor OOB values (no OOB setting needed) - &[DefaultParam { + const PARAMS: &[ParamDefinition] = &[ParamDefinition { name: "reverse", default: DefaultParamValue::Boolean(false), - }] + constraint: ParamConstraint::boolean(), + }]; + PARAMS } fn allowed_transforms(&self) -> &'static [TransformKind] { @@ -102,14 +104,10 @@ impl ScaleTypeTrait for Discrete { return Ok(t.clone()); } else { return Err(format!( - "Transform '{}' not supported for {} scale. Allowed: {}", - t.name(), + "{} scale transform should be {}, not '{}'", self.name(), - self.allowed_transforms() - .iter() - .map(|k| k.to_string()) - .collect::>() - .join(", ") + crate::or_list(self.allowed_transforms()), + t.name() )); } } @@ -456,9 +454,7 @@ mod tests { let result = discrete.resolve_transform("color", Some(&log_transform), None, None); assert!(result.is_err()); - assert!(result - .unwrap_err() - .contains("not supported for discrete scale")); + assert!(result.unwrap_err().contains("not 'log'")); } // ========================================================================= diff --git a/src/plot/scale/scale_type/mod.rs b/src/plot/scale/scale_type/mod.rs index 4fa345f9..2d397e39 100644 --- a/src/plot/scale/scale_type/mod.rs +++ b/src/plot/scale/scale_type/mod.rs @@ -27,7 +27,7 @@ use std::sync::Arc; use super::transform::{Transform, TransformKind}; use crate::plot::aesthetic::{is_facet_aesthetic, is_positional_aesthetic}; -use crate::plot::types::{DefaultParam, DefaultParamValue}; +use crate::plot::types::{validate_parameter, ParamDefinition, DefaultParamValue}; use crate::plot::{ArrayElement, ColumnInfo, ParameterValue}; // Scale type implementations @@ -547,7 +547,7 @@ pub trait ScaleTypeTrait: std::fmt::Debug + std::fmt::Display + Send + Sync { /// default value. The `resolve_properties()` method handles these special cases. /// /// Default: empty (no properties allowed). - fn default_properties(&self) -> &'static [DefaultParam] { + fn default_properties(&self) -> &'static [ParamDefinition] { &[] } @@ -609,14 +609,10 @@ pub trait ScaleTypeTrait: std::fmt::Debug + std::fmt::Display + Send + Sync { Ok(t.clone()) } else { Err(format!( - "Transform '{}' not supported for {} scale. Allowed: {}", - t.name(), + "{} scale transform should be {}, not '{}'", self.name(), - self.allowed_transforms() - .iter() - .map(|k| k.to_string()) - .collect::>() - .join(", ") + crate::or_list(self.allowed_transforms()), + t.name() )) } } @@ -634,29 +630,40 @@ pub trait ScaleTypeTrait: std::fmt::Debug + std::fmt::Display + Send + Sync { let defaults = self.default_properties(); let is_positional = is_positional_aesthetic(aesthetic); - // Build allowed list, excluding "expand" for non-positional aesthetics - let allowed: Vec<&str> = defaults - .iter() - .filter(|p| p.name != "expand" || is_positional) - .map(|p| p.name) - .collect(); - - // Check for unknown properties - for key in properties.keys() { - if !allowed.contains(&key.as_str()) { - if allowed.is_empty() { - return Err(format!( - "{} scale does not support any SETTING properties", - self.name() - )); + // Validate values against constraints + for (key, value) in properties.iter() { + // Check if property is allowed for this aesthetic type + let is_expand_on_non_positional = key == "expand" && !is_positional; + + // Find the parameter definition and validate if allowed + let param = defaults.iter().find(|p| p.name == key); + if let Some(param) = param { + if !is_expand_on_non_positional { + validate_parameter(key, value, ¶m.constraint)?; + continue; } - return Err(format!( - "{} scale does not support SETTING '{}'. Allowed: {}", - self.name(), - key, - allowed.join(", ") - )); } + + // Property not found or not allowed for this aesthetic type + let allowed: Vec<&str> = defaults + .iter() + .filter(|p| p.name != "expand" || is_positional) + .map(|p| p.name) + .collect(); + return Err(if allowed.is_empty() { + format!( + "{} scale does not accept any properties, but got '{}'", + self.name(), + key + ) + } else { + format!( + "{} scale property should be {}, not '{}'", + self.name(), + crate::or_list_quoted(&allowed, '\''), + key + ) + }); } // Start with user properties, add defaults for missing ones @@ -680,10 +687,8 @@ pub trait ScaleTypeTrait: std::fmt::Debug + std::fmt::Display + Send + Sync { } } - // Validate oob value if present + // Additional semantic validation for oob (scale-specific restrictions beyond the constraint) if let Some(ParameterValue::String(oob)) = resolved.get("oob") { - validate_oob(oob)?; - // Discrete and Ordinal scales only support "censor" - no way to map unmapped values to output let kind = self.scale_type_kind(); if (kind == ScaleTypeKind::Discrete || kind == ScaleTypeKind::Ordinal) @@ -696,16 +701,6 @@ pub trait ScaleTypeTrait: std::fmt::Debug + std::fmt::Display + Send + Sync { oob )); } - - // Binned scales support "censor" and "squish", but not "keep" - // Values outside bins have no bin to map to, but can be squished to nearest bin edge - if kind == ScaleTypeKind::Binned && oob == OOB_KEEP { - return Err(format!( - "{} scale does not support oob='keep'. Use 'censor' to exclude values \ - outside bins, or 'squish' to clamp them to the nearest bin edge.", - self.name() - )); - } } Ok(resolved) @@ -1387,6 +1382,13 @@ pub const OOB_SQUISH: &str = "squish"; /// Out-of-bounds mode: don't modify values (default for positional aesthetics) pub const OOB_KEEP: &str = "keep"; +/// Valid OOB values for continuous scales (all three options) +pub const OOB_VALUES_CONTINUOUS: &[&str] = &[OOB_CENSOR, OOB_SQUISH, OOB_KEEP]; +/// Valid OOB values for binned scales (keep is not supported) +pub const OOB_VALUES_BINNED: &[&str] = &[OOB_CENSOR, OOB_SQUISH]; +/// Valid closed side values for binned data +pub const CLOSED_VALUES: &[&str] = &["left", "right"]; + /// Default oob mode for an aesthetic. /// Positional aesthetics default to "keep", others default to "censor". pub fn default_oob(aesthetic: &str) -> &'static str { @@ -1397,17 +1399,6 @@ pub fn default_oob(aesthetic: &str) -> &'static str { } } -/// Validate oob value is one of the allowed modes. -pub(super) fn validate_oob(value: &str) -> Result<(), String> { - match value { - OOB_CENSOR | OOB_SQUISH | OOB_KEEP => Ok(()), - _ => Err(format!( - "Invalid oob value '{}'. Must be 'censor', 'squish', or 'keep'", - value - )), - } -} - // ============================================================================= // Output range resolution helpers // ============================================================================= @@ -2338,9 +2329,7 @@ mod tests { expand_props.insert("expand".to_string(), ParameterValue::Number(0.1)); let result = ScaleType::discrete().resolve_properties("color", &expand_props); assert!(result.is_err()); - assert!(result - .unwrap_err() - .contains("does not support SETTING 'expand'")); + assert!(result.unwrap_err().contains("not 'expand'")); // Identity rejects any property let result = ScaleType::identity().resolve_properties("x", &expand_props); @@ -2354,7 +2343,12 @@ mod tests { ); let result = ScaleType::binned().resolve_properties("x", &keep_props); assert!(result.is_err()); - assert!(result.unwrap_err().contains("does not support oob='keep'")); + let err = result.unwrap_err(); + assert!( + err.contains("not 'keep'"), + "Expected error about invalid oob value, got: {}", + err + ); } #[test] @@ -2519,7 +2513,11 @@ mod tests { let result = ScaleType::continuous().resolve_properties("x", &props); assert!(result.is_err()); let err = result.unwrap_err(); - assert!(err.contains("Invalid oob value")); + assert!( + err.contains("not 'invalid'"), + "Expected error about invalid oob value, got: {}", + err + ); } #[test] @@ -2557,9 +2555,7 @@ mod tests { .is_err()); let result = ScaleType::discrete().resolve_properties("color", &oob_props); assert!(result.is_err()); - assert!(result - .unwrap_err() - .contains("does not support SETTING 'oob'")); + assert!(result.unwrap_err().contains("not 'oob'")); // Discrete has no oob in resolved (implicit censor) let resolved = ScaleType::discrete() @@ -2668,7 +2664,7 @@ mod tests { let log = Transform::log(); let result = ScaleType::discrete().resolve_transform("color", Some(&log), None, None); assert!(result.is_err()); - assert!(result.unwrap_err().contains("not supported")); + assert!(result.unwrap_err().contains("not 'log'")); // Discrete accepts string and bool for (transform, expected_kind) in [ @@ -2923,9 +2919,7 @@ mod tests { let result = ScaleType::discrete().resolve_properties("x", &props); assert!(result.is_err()); - assert!(result - .unwrap_err() - .contains("does not support SETTING 'breaks'")); + assert!(result.unwrap_err().contains("not 'breaks'")); } #[test] diff --git a/src/plot/scale/scale_type/ordinal.rs b/src/plot/scale/scale_type/ordinal.rs index a315af14..e0121465 100644 --- a/src/plot/scale/scale_type/ordinal.rs +++ b/src/plot/scale/scale_type/ordinal.rs @@ -8,7 +8,7 @@ use polars::prelude::DataType; use super::super::transform::{Transform, TransformKind}; use super::{ScaleTypeKind, ScaleTypeTrait}; -use crate::plot::types::{DefaultParam, DefaultParamValue}; +use crate::plot::types::{ParamConstraint, ParamDefinition, DefaultParamValue}; use crate::plot::ArrayElement; /// Ordinal scale type - for ordered categorical data with interpolated output @@ -113,14 +113,10 @@ impl ScaleTypeTrait for Ordinal { return Ok(t.clone()); } else { return Err(format!( - "Transform '{}' not supported for {} scale. Allowed: {}", - t.name(), + "{} scale transform should be {}, not '{}'", self.name(), - self.allowed_transforms() - .iter() - .map(|k| k.to_string()) - .collect::>() - .join(", ") + crate::or_list(self.allowed_transforms()), + t.name() )); } } @@ -138,12 +134,14 @@ impl ScaleTypeTrait for Ordinal { )) } - fn default_properties(&self) -> &'static [DefaultParam] { + fn default_properties(&self) -> &'static [ParamDefinition] { // Ordinal scales always censor OOB values (no OOB setting needed) - &[DefaultParam { + const PARAMS: &[ParamDefinition] = &[ParamDefinition { name: "reverse", default: DefaultParamValue::Boolean(false), - }] + constraint: ParamConstraint::boolean(), + }]; + PARAMS } fn default_output_range( diff --git a/src/plot/types.rs b/src/plot/types.rs index c0639868..0ff86aba 100644 --- a/src/plot/types.rs +++ b/src/plot/types.rs @@ -1068,17 +1068,564 @@ pub enum DefaultParamValue { Null, } -/// Property definition: name and default value +// ============================================================================= +// Parameter Constraint Types +// ============================================================================= + +/// Constraint state for a specific type +#[derive(Debug, Clone, Copy)] +pub enum TypeConstraint { + /// Type not allowed for this parameter + Forbidden, + /// Type allowed, any value accepted + Any, + /// Type allowed with specific constraints + Constrained(T), +} + +/// Constraint for Number values +#[derive(Debug, Clone, Copy)] +pub struct NumberConstraint { + pub min: Option, + pub max: Option, + pub min_exclusive: bool, + pub max_exclusive: bool, + pub whole: bool, +} + +impl NumberConstraint { + /// No constraints on value + pub const fn unconstrained() -> Self { + Self { + min: None, + max: None, + min_exclusive: false, + max_exclusive: false, + whole: false, + } + } + + /// Minimum value (inclusive) + pub const fn min(min: f64) -> Self { + Self { + min: Some(min), + max: None, + min_exclusive: false, + max_exclusive: false, + whole: false, + } + } + + /// Minimum value (exclusive) + pub const fn min_exclusive(min: f64) -> Self { + Self { + min: Some(min), + max: None, + min_exclusive: true, + max_exclusive: false, + whole: false, + } + } + + /// Value within range (inclusive on both ends) + pub const fn range(min: f64, max: f64) -> Self { + Self { + min: Some(min), + max: Some(max), + min_exclusive: false, + max_exclusive: false, + whole: false, + } + } + + /// Whole number (integer) with minimum value (inclusive) + pub const fn count(min: f64) -> Self { + Self { + min: Some(min), + max: None, + min_exclusive: false, + max_exclusive: false, + whole: true, + } + } + + /// Whole number (integer) within range (inclusive on both ends) + pub const fn count_range(min: f64, max: f64) -> Self { + Self { + min: Some(min), + max: Some(max), + min_exclusive: false, + max_exclusive: false, + whole: true, + } + } +} + +/// Constraint for String values +#[derive(Debug, Clone, Copy)] +pub struct StringConstraint { + /// String must be one of these values (empty = any string allowed) + pub allowed_values: &'static [&'static str], +} + +impl StringConstraint { + /// Any string allowed (empty slice = no restriction) + pub const fn unconstrained() -> Self { + Self { + allowed_values: &[], + } + } + + /// String must be one of the specified values + pub const fn one_of(values: &'static [&'static str]) -> Self { + Self { + allowed_values: values, + } + } +} + +/// Element constraint - specifies type AND value constraints for array elements +#[derive(Debug, Clone, Copy)] +pub enum ArrayElementConstraint { + /// Any element type allowed, no constraints + Any, + /// Must be numbers, with optional value constraint + Number(NumberConstraint), + /// Must be strings, with optional value constraint + String(StringConstraint), + /// Must be booleans + Boolean, +} + +/// Constraint for Array values +#[derive(Debug, Clone, Copy)] +pub struct ArrayConstraint { + pub element: ArrayElementConstraint, + pub min_len: Option, + pub max_len: Option, + pub allow_null_elements: bool, +} + +impl ArrayConstraint { + /// Array of numbers with value constraint + pub const fn of_numbers(constraint: NumberConstraint) -> Self { + Self { + element: ArrayElementConstraint::Number(constraint), + min_len: None, + max_len: None, + allow_null_elements: false, + } + } + + /// Array of numbers with exact length and value constraint + pub const fn of_numbers_len(constraint: NumberConstraint, len: usize) -> Self { + Self { + element: ArrayElementConstraint::Number(constraint), + min_len: Some(len), + max_len: Some(len), + allow_null_elements: false, + } + } + + /// Array of strings with value constraint + pub const fn of_strings(constraint: StringConstraint) -> Self { + Self { + element: ArrayElementConstraint::String(constraint), + min_len: None, + max_len: None, + allow_null_elements: false, + } + } + + /// Array of strings with exact length and value constraint + #[allow(dead_code)] + pub const fn of_strings_len(constraint: StringConstraint, len: usize) -> Self { + Self { + element: ArrayElementConstraint::String(constraint), + min_len: Some(len), + max_len: Some(len), + allow_null_elements: false, + } + } + + /// Any element types allowed (including nulls) + #[allow(dead_code)] + pub const fn any_elements() -> Self { + Self { + element: ArrayElementConstraint::Any, + min_len: None, + max_len: None, + allow_null_elements: true, + } + } + + /// Builder method to allow null elements + #[allow(dead_code)] + pub const fn with_null_elements(mut self) -> Self { + self.allow_null_elements = true; + self + } +} + +/// Validation constraint for a parameter value. +/// +/// Each field specifies whether a type is forbidden, allowed (any value), or constrained. +/// Used by `ParamDefinition` to specify validation rules for parameters. +#[derive(Debug, Clone, Copy)] +pub struct ParamConstraint { + pub number: TypeConstraint, + pub string: TypeConstraint, + pub boolean: TypeConstraint<()>, + pub array: TypeConstraint, + pub allow_null: bool, +} + +impl ParamConstraint { + /// All types allowed, no constraints (default) + pub const fn unconstrained() -> Self { + Self { + number: TypeConstraint::Any, + string: TypeConstraint::Any, + boolean: TypeConstraint::Any, + array: TypeConstraint::Any, + allow_null: true, + } + } + + /// Number only with constraint + pub const fn number(constraint: NumberConstraint) -> Self { + Self { + number: TypeConstraint::Constrained(constraint), + string: TypeConstraint::Forbidden, + boolean: TypeConstraint::Forbidden, + array: TypeConstraint::Forbidden, + allow_null: true, + } + } + + /// Number only, any value + #[allow(dead_code)] + pub const fn number_any() -> Self { + Self { + number: TypeConstraint::Any, + string: TypeConstraint::Forbidden, + boolean: TypeConstraint::Forbidden, + array: TypeConstraint::Forbidden, + allow_null: true, + } + } + + /// String only with enum constraint + pub const fn string_option(values: &'static [&'static str]) -> Self { + Self { + number: TypeConstraint::Forbidden, + string: TypeConstraint::Constrained(StringConstraint::one_of(values)), + boolean: TypeConstraint::Forbidden, + array: TypeConstraint::Forbidden, + allow_null: true, + } + } + + /// String only + pub const fn string() -> Self { + Self { + number: TypeConstraint::Forbidden, + string: TypeConstraint::Any, + boolean: TypeConstraint::Forbidden, + array: TypeConstraint::Forbidden, + allow_null: true, + } + } + + /// Boolean only + pub const fn boolean() -> Self { + Self { + number: TypeConstraint::Forbidden, + string: TypeConstraint::Forbidden, + boolean: TypeConstraint::Any, + array: TypeConstraint::Forbidden, + allow_null: true, + } + } + + /// Number or Array of numbers - for `expand` parameter + pub const fn number_or_numeric_array(num: NumberConstraint, arr: ArrayConstraint) -> Self { + Self { + number: TypeConstraint::Constrained(num), + string: TypeConstraint::Forbidden, + boolean: TypeConstraint::Forbidden, + array: TypeConstraint::Constrained(arr), + allow_null: true, + } + } + + /// Number, Array of numbers, or String (any) - for `breaks` parameter + pub const fn number_or_array_or_string(num: NumberConstraint, arr: ArrayConstraint) -> Self { + Self { + number: TypeConstraint::Constrained(num), + string: TypeConstraint::Any, // Any string (temporal interval validated elsewhere) + boolean: TypeConstraint::Forbidden, + array: TypeConstraint::Constrained(arr), + allow_null: true, + } + } + + /// String enum or Array of strings from same enum - for `free` parameter + #[allow(dead_code)] + pub const fn string_or_string_array(values: &'static [&'static str]) -> Self { + Self { + number: TypeConstraint::Forbidden, + string: TypeConstraint::Constrained(StringConstraint::one_of(values)), + boolean: TypeConstraint::Forbidden, + array: TypeConstraint::Constrained(ArrayConstraint::of_strings( + StringConstraint::one_of(values), + )), + allow_null: true, + } + } + + /// Builder method to disallow null values + #[allow(dead_code)] + pub const fn required(mut self) -> Self { + self.allow_null = false; + self + } + + // ========================================================================= + // Convenience constructors + // ========================================================================= + + /// Create a constraint requiring a minimum value (inclusive) + pub const fn number_min(min: f64) -> Self { + Self::number(NumberConstraint::min(min)) + } + + /// Create a constraint requiring a minimum value (exclusive) + pub const fn number_min_exclusive(min: f64) -> Self { + Self::number(NumberConstraint::min_exclusive(min)) + } + + /// Create a constraint requiring a value within a range (inclusive on both ends) + pub const fn number_range(min: f64, max: f64) -> Self { + Self::number(NumberConstraint::range(min, max)) + } + + /// Create a constraint requiring a whole number (integer) with minimum value + pub const fn count(min: f64) -> Self { + Self::number(NumberConstraint::count(min)) + } + + /// Create a constraint requiring a whole number (integer) within a range + pub const fn count_range(min: f64, max: f64) -> Self { + Self::number(NumberConstraint::count_range(min, max)) + } +} + +/// Validate a parameter value against a constraint. +/// +/// Returns Ok(()) if the value passes validation, or Err with a descriptive +/// error message if validation fails. +pub fn validate_parameter( + param_name: &str, + value: &ParameterValue, + constraint: &ParamConstraint, +) -> Result<(), String> { + match value { + ParameterValue::Number(n) => match &constraint.number { + TypeConstraint::Forbidden => { + Err(type_not_allowed_error(param_name, "Number", constraint)) + } + TypeConstraint::Any => Ok(()), + TypeConstraint::Constrained(c) => validate_number(param_name, *n, c), + }, + ParameterValue::String(s) => match &constraint.string { + TypeConstraint::Forbidden => { + Err(type_not_allowed_error(param_name, "String", constraint)) + } + TypeConstraint::Any => Ok(()), + TypeConstraint::Constrained(c) => validate_string(param_name, s, c), + }, + ParameterValue::Boolean(_) => match &constraint.boolean { + TypeConstraint::Forbidden => { + Err(type_not_allowed_error(param_name, "Boolean", constraint)) + } + TypeConstraint::Any | TypeConstraint::Constrained(()) => Ok(()), + }, + ParameterValue::Array(arr) => match &constraint.array { + TypeConstraint::Forbidden => { + Err(type_not_allowed_error(param_name, "Array", constraint)) + } + TypeConstraint::Any => Ok(()), + TypeConstraint::Constrained(c) => validate_array(param_name, arr, c), + }, + ParameterValue::Null => { + if constraint.allow_null { + Ok(()) + } else { + Err(format!( + "Parameter '{}' is required (cannot be null)", + param_name + )) + } + } + } +} + +fn validate_number(name: &str, n: f64, c: &NumberConstraint) -> Result<(), String> { + // Check whole number constraint first + if c.whole && n.fract() != 0.0 { + return Err(format!("'{}' should be a whole number, not {}", name, n)); + } + if let Some(min) = c.min { + let ok = if c.min_exclusive { n > min } else { n >= min }; + if !ok { + let op = if c.min_exclusive { ">" } else { ">=" }; + return Err(format!("'{}' should be {} {}, not {}", name, op, min, n)); + } + } + if let Some(max) = c.max { + let ok = if c.max_exclusive { n < max } else { n <= max }; + if !ok { + let op = if c.max_exclusive { "<" } else { "<=" }; + return Err(format!("'{}' should be {} {}, not {}", name, op, max, n)); + } + } + Ok(()) +} + +fn validate_string(name: &str, s: &str, c: &StringConstraint) -> Result<(), String> { + // Empty allowed_values = unconstrained (any string) + if c.allowed_values.is_empty() { + return Ok(()); + } + if !c.allowed_values.contains(&s) { + return Err(format!( + "'{}' should be {}, not '{}'", + name, + crate::or_list_quoted(c.allowed_values, '\''), + s + )); + } + Ok(()) +} + +fn validate_array(name: &str, arr: &[ArrayElement], c: &ArrayConstraint) -> Result<(), String> { + // Length validation + let len = arr.len(); + match (c.min_len, c.max_len) { + // Exact length required + (Some(min), Some(max)) if min == max && len != min => { + return Err(format!( + "Parameter '{}' array must have exactly {} element(s) (got {})", + name, min, len + )); + } + // Range constraint (both bounds, not equal) + (Some(min), Some(max)) if len < min || len > max => { + return Err(format!( + "Parameter '{}' array must have between {} and {} element(s) (got {})", + name, min, max, len + )); + } + // One-sided constraints + (Some(min), None) if len < min => { + return Err(format!( + "Parameter '{}' array must have at least {} element(s) (got {})", + name, min, len + )); + } + (None, Some(max)) if len > max => { + return Err(format!( + "Parameter '{}' array must have at most {} element(s) (got {})", + name, max, len + )); + } + _ => {} + } + // Element type and value validation - collect all errors + let mut errors: Vec = Vec::new(); + for (i, elem) in arr.iter().enumerate() { + match (&c.element, elem) { + // Handle null elements + (_, ArrayElement::Null) if c.allow_null_elements => {} + (_, ArrayElement::Null) => { + errors.push(format!("'{}[{}]' cannot be null", name, i)); + } + // Any element type allowed + (ArrayElementConstraint::Any, _) => {} + // Number elements + (ArrayElementConstraint::Number(nc), ArrayElement::Number(n)) => { + if let Err(e) = validate_number(&format!("{}[{}]", name, i), *n, nc) { + errors.push(e); + } + } + (ArrayElementConstraint::Number(_), _) => { + errors.push(format!("'{}[{}]' must be a number", name, i)); + } + // String elements + (ArrayElementConstraint::String(sc), ArrayElement::String(s)) => { + // Only validate if allowed_values is non-empty (empty = any string) + if !sc.allowed_values.is_empty() { + if let Err(e) = validate_string(&format!("{}[{}]", name, i), s, sc) { + errors.push(e); + } + } + } + (ArrayElementConstraint::String(_), _) => { + errors.push(format!("'{}[{}]' must be a string", name, i)); + } + // Boolean elements + (ArrayElementConstraint::Boolean, ArrayElement::Boolean(_)) => {} + (ArrayElementConstraint::Boolean, _) => { + errors.push(format!("'{}[{}]' must be a boolean", name, i)); + } + } + } + if errors.is_empty() { + Ok(()) + } else { + Err(format!( + "Parameter '{}' has invalid elements:\n— {}", + name, + errors.join("\n— ") + )) + } +} + +fn type_not_allowed_error(name: &str, got: &str, c: &ParamConstraint) -> String { + let mut allowed = Vec::new(); + if !matches!(c.number, TypeConstraint::Forbidden) { + allowed.push("Number"); + } + if !matches!(c.string, TypeConstraint::Forbidden) { + allowed.push("String"); + } + if !matches!(c.boolean, TypeConstraint::Forbidden) { + allowed.push("Boolean"); + } + if !matches!(c.array, TypeConstraint::Forbidden) { + allowed.push("Array"); + } + format!( + "'{}' should be {}, not {}", + name, + crate::or_list(&allowed), + got + ) +} + +/// Property definition: name, default value, and validation constraint. /// /// Used by `CoordTrait`, `ScaleTypeTrait`, and `GeomTrait` to declare their /// allowed properties and default values in a single place. #[derive(Debug, Clone)] -pub struct DefaultParam { +pub struct ParamDefinition { pub name: &'static str, pub default: DefaultParamValue, + pub constraint: ParamConstraint, } -impl DefaultParam { +impl ParamDefinition { /// Convert the default value to a ParameterValue, if not Null pub fn to_parameter_value(&self) -> Option { match &self.default { @@ -1527,6 +2074,316 @@ mod tests { assert!(result.is_err()); } + // ========================================================================= + // ParamConstraint validation tests + // ========================================================================= + + #[test] + fn test_number_constraint_accepts_valid() { + let constraint = ParamConstraint::number_min(0.0); + assert!(validate_parameter("test", &ParameterValue::Number(5.0), &constraint).is_ok()); + assert!(validate_parameter("test", &ParameterValue::Number(0.0), &constraint).is_ok()); + } + + #[test] + fn test_number_constraint_rejects_invalid() { + let constraint = ParamConstraint::number_min(0.0); + let result = validate_parameter("test", &ParameterValue::Number(-1.0), &constraint); + assert!(result.is_err()); + assert!(result.unwrap_err().contains(">= 0")); + } + + #[test] + fn test_number_constraint_rejects_wrong_type() { + let constraint = ParamConstraint::number_min(0.0); + let result = validate_parameter( + "test", + &ParameterValue::String("hello".to_string()), + &constraint, + ); + assert!(result.is_err()); + assert!(result.unwrap_err().contains("should be Number")); + } + + #[test] + fn test_count_constraint_accepts_whole() { + let constraint = ParamConstraint::count(1.0); + assert!(validate_parameter("bins", &ParameterValue::Number(5.0), &constraint).is_ok()); + assert!(validate_parameter("bins", &ParameterValue::Number(1.0), &constraint).is_ok()); + assert!(validate_parameter("bins", &ParameterValue::Number(100.0), &constraint).is_ok()); + } + + #[test] + fn test_count_constraint_rejects_fractional() { + let constraint = ParamConstraint::count(1.0); + let result = validate_parameter("bins", &ParameterValue::Number(5.5), &constraint); + assert!(result.is_err()); + assert!(result.unwrap_err().contains("whole number")); + } + + #[test] + fn test_count_constraint_rejects_below_min() { + let constraint = ParamConstraint::count(1.0); + let result = validate_parameter("bins", &ParameterValue::Number(0.0), &constraint); + assert!(result.is_err()); + assert!(result.unwrap_err().contains(">= 1")); + } + + #[test] + fn test_string_option_accepts_valid() { + let constraint = ParamConstraint::string_option(&["a", "b", "c"]); + assert!(validate_parameter( + "test", + &ParameterValue::String("a".to_string()), + &constraint + ) + .is_ok()); + } + + #[test] + fn test_string_option_rejects_invalid() { + let constraint = ParamConstraint::string_option(&["a", "b", "c"]); + let result = validate_parameter( + "test", + &ParameterValue::String("d".to_string()), + &constraint, + ); + assert!(result.is_err()); + assert!(result.unwrap_err().contains("not 'd'")); + } + + #[test] + fn test_string_option_rejects_wrong_type() { + let constraint = ParamConstraint::string_option(&["a", "b", "c"]); + let result = validate_parameter("test", &ParameterValue::Number(1.0), &constraint); + assert!(result.is_err()); + assert!(result.unwrap_err().contains("should be String")); + } + + #[test] + fn test_boolean_accepts_valid() { + let constraint = ParamConstraint::boolean(); + assert!(validate_parameter("reverse", &ParameterValue::Boolean(true), &constraint).is_ok()); + assert!( + validate_parameter("reverse", &ParameterValue::Boolean(false), &constraint).is_ok() + ); + } + + #[test] + fn test_boolean_rejects_wrong_type() { + let constraint = ParamConstraint::boolean(); + let result = validate_parameter( + "reverse", + &ParameterValue::String("true".to_string()), + &constraint, + ); + assert!(result.is_err()); + assert!(result.unwrap_err().contains("should be Boolean")); + } + + #[test] + fn test_multi_type_number_or_array_accepts_number() { + let constraint = ParamConstraint::number_or_numeric_array( + NumberConstraint::min(0.0), + ArrayConstraint::of_numbers_len(NumberConstraint::unconstrained(), 2), + ); + assert!(validate_parameter("expand", &ParameterValue::Number(0.05), &constraint).is_ok()); + } + + #[test] + fn test_multi_type_number_or_array_accepts_array() { + let constraint = ParamConstraint::number_or_numeric_array( + NumberConstraint::min(0.0), + ArrayConstraint::of_numbers_len(NumberConstraint::min(0.0), 2), + ); + let arr = + ParameterValue::Array(vec![ArrayElement::Number(0.05), ArrayElement::Number(10.0)]); + assert!(validate_parameter("expand", &arr, &constraint).is_ok()); + } + + #[test] + fn test_multi_type_number_or_array_rejects_wrong_array_length() { + let constraint = ParamConstraint::number_or_numeric_array( + NumberConstraint::min(0.0), + ArrayConstraint::of_numbers_len(NumberConstraint::min(0.0), 2), + ); + let arr = ParameterValue::Array(vec![ArrayElement::Number(0.05)]); + let result = validate_parameter("expand", &arr, &constraint); + assert!(result.is_err()); + assert!(result.unwrap_err().contains("exactly 2")); + } + + #[test] + fn test_multi_type_number_or_array_rejects_string() { + let constraint = ParamConstraint::number_or_numeric_array( + NumberConstraint::min(0.0), + ArrayConstraint::of_numbers_len(NumberConstraint::min(0.0), 2), + ); + let result = validate_parameter( + "expand", + &ParameterValue::String("0.05".to_string()), + &constraint, + ); + assert!(result.is_err()); + assert!(result.unwrap_err().contains("should be Number or Array")); + } + + #[test] + fn test_multi_type_number_or_array_validates_element_values() { + let constraint = ParamConstraint::number_or_numeric_array( + NumberConstraint::min(0.0), + ArrayConstraint::of_numbers_len(NumberConstraint::min(0.0), 2), + ); + let arr = ParameterValue::Array(vec![ + ArrayElement::Number(0.05), + ArrayElement::Number(-10.0), // Invalid: negative + ]); + let result = validate_parameter("expand", &arr, &constraint); + assert!(result.is_err()); + let err = result.unwrap_err(); + assert!(err.contains("expand[1]")); + assert!(err.contains(">= 0")); + } + + #[test] + fn test_breaks_constraint_accepts_number() { + let constraint = ParamConstraint::number_or_array_or_string( + NumberConstraint::min(1.0), + ArrayConstraint::of_numbers(NumberConstraint::unconstrained()), + ); + assert!(validate_parameter("breaks", &ParameterValue::Number(10.0), &constraint).is_ok()); + } + + #[test] + fn test_breaks_constraint_accepts_array() { + let constraint = ParamConstraint::number_or_array_or_string( + NumberConstraint::min(1.0), + ArrayConstraint::of_numbers(NumberConstraint::unconstrained()), + ); + let arr = ParameterValue::Array(vec![ + ArrayElement::Number(0.0), + ArrayElement::Number(25.0), + ArrayElement::Number(50.0), + ]); + assert!(validate_parameter("breaks", &arr, &constraint).is_ok()); + } + + #[test] + fn test_breaks_constraint_accepts_string() { + let constraint = ParamConstraint::number_or_array_or_string( + NumberConstraint::min(1.0), + ArrayConstraint::of_numbers(NumberConstraint::unconstrained()), + ); + assert!(validate_parameter( + "breaks", + &ParameterValue::String("1 month".to_string()), + &constraint + ) + .is_ok()); + } + + #[test] + fn test_breaks_constraint_rejects_boolean() { + let constraint = ParamConstraint::number_or_array_or_string( + NumberConstraint::min(1.0), + ArrayConstraint::of_numbers(NumberConstraint::unconstrained()), + ); + let result = validate_parameter("breaks", &ParameterValue::Boolean(true), &constraint); + assert!(result.is_err()); + assert!(result + .unwrap_err() + .contains("should be Number, String, or Array")); + } + + #[test] + fn test_breaks_constraint_validates_number_value() { + let constraint = ParamConstraint::number_or_array_or_string( + NumberConstraint::min(1.0), + ArrayConstraint::of_numbers(NumberConstraint::unconstrained()), + ); + let result = validate_parameter("breaks", &ParameterValue::Number(0.0), &constraint); + assert!(result.is_err()); + assert!(result.unwrap_err().contains(">= 1")); + } + + #[test] + fn test_array_element_type_validation() { + let constraint = ParamConstraint::number_or_numeric_array( + NumberConstraint::min(0.0), + ArrayConstraint::of_numbers(NumberConstraint::unconstrained()), + ); + let arr = ParameterValue::Array(vec![ + ArrayElement::Number(1.0), + ArrayElement::String("fifty".to_string()), + ArrayElement::Number(100.0), + ]); + let result = validate_parameter("breaks", &arr, &constraint); + assert!(result.is_err()); + assert!(result.unwrap_err().contains("'breaks[1]' must be a number")); + } + + #[test] + fn test_null_allowed_by_default() { + let constraint = ParamConstraint::number_min(1.0); + assert!(validate_parameter("test", &ParameterValue::Null, &constraint).is_ok()); + } + + #[test] + fn test_null_rejected_when_required() { + let constraint = ParamConstraint::number_min(1.0).required(); + let result = validate_parameter("test", &ParameterValue::Null, &constraint); + assert!(result.is_err()); + assert!(result.unwrap_err().contains("required")); + } + + #[test] + fn test_array_null_elements_rejected_by_default() { + let constraint = ParamConstraint::number_or_numeric_array( + NumberConstraint::min(0.0), + ArrayConstraint::of_numbers(NumberConstraint::unconstrained()), + ); + let arr = ParameterValue::Array(vec![ + ArrayElement::Number(1.0), + ArrayElement::Null, + ArrayElement::Number(3.0), + ]); + let result = validate_parameter("values", &arr, &constraint); + assert!(result.is_err()); + assert!(result.unwrap_err().contains("'values[1]' cannot be null")); + } + + #[test] + fn test_string_or_string_array_accepts_string() { + let constraint = ParamConstraint::string_or_string_array(&["x", "y"]); + assert!(validate_parameter( + "free", + &ParameterValue::String("x".to_string()), + &constraint + ) + .is_ok()); + } + + #[test] + fn test_string_or_string_array_accepts_array() { + let constraint = ParamConstraint::string_or_string_array(&["x", "y"]); + let arr = ParameterValue::Array(vec![ + ArrayElement::String("x".to_string()), + ArrayElement::String("y".to_string()), + ]); + assert!(validate_parameter("free", &arr, &constraint).is_ok()); + } + + #[test] + fn test_string_or_string_array_validates_array_elements() { + let constraint = ParamConstraint::string_or_string_array(&["x", "y"]); + let arr = ParameterValue::Array(vec![ + ArrayElement::String("x".to_string()), + ArrayElement::String("z".to_string()), + ]); + let result = validate_parameter("free", &arr, &constraint); + assert!(result.is_err()); + assert!(result.unwrap_err().contains("not 'z'")); + } #[test] fn test_homogenize_mixed_number_string() { let arr = vec![ diff --git a/src/util.rs b/src/util.rs new file mode 100644 index 00000000..b1ac3d36 --- /dev/null +++ b/src/util.rs @@ -0,0 +1,230 @@ +//! General-purpose utility functions for consistent string formatting across the codebase. + +use std::fmt::Display; + +/// Format a list of items with "and", following Oxford comma rules. +/// +/// Works with any type implementing `Display` (strings, enums, numbers, etc.) +/// +/// # Examples +/// - `and_list(&["a", "b", "c"])` → `a, b, and c` +/// - `and_list(&[Color::Red, Color::Green])` → `red and green` +/// - `and_list(&[1, 2, 3])` → `1, 2, and 3` +pub fn and_list(items: &[T]) -> String { + format_list(items, "and") +} + +/// Format a list of items with "or", following Oxford comma rules. +/// +/// Works with any type implementing `Display` (strings, enums, numbers, etc.) +/// +/// # Examples +/// - `or_list(&["a", "b", "c"])` → `a, b, or c` +/// - `or_list(&[Color::Red, Color::Green])` → `red or green` +pub fn or_list(items: &[T]) -> String { + format_list(items, "or") +} + +fn format_list(items: &[T], conjunction: &str) -> String { + match items.len() { + 0 => String::new(), + 1 => items[0].to_string(), + 2 => format!("{} {} {}", items[0], conjunction, items[1]), + _ => { + let mut result = String::new(); + for (i, item) in items.iter().enumerate() { + if i > 0 { + result.push_str(", "); + } + if i == items.len() - 1 { + result.push_str(conjunction); + result.push(' '); + } + result.push_str(&item.to_string()); + } + result + } + } +} + +/// Format a list of items with quotes and "and", following Oxford comma rules. +/// +/// Works with any type implementing `Display` (strings, enums, numbers, etc.) +/// +/// # Examples +/// - `and_list_quoted(&["a", "b", "c"], '\'')` → `'a', 'b', and 'c'` +/// - `and_list_quoted(&[Color::Red, Color::Green], '\'')` → `'red' and 'green'` +pub fn and_list_quoted(items: &[T], quote: char) -> String { + format_quoted_list(items, quote, "and") +} + +/// Format a list of items with quotes and "or", following Oxford comma rules. +/// +/// Works with any type implementing `Display` (strings, enums, numbers, etc.) +/// +/// # Examples +/// - `or_list_quoted(&["a", "b", "c"], '\'')` → `'a', 'b', or 'c'` +/// - `or_list_quoted(&[Color::Red, Color::Green], '\'')` → `'red' or 'green'` +pub fn or_list_quoted(items: &[T], quote: char) -> String { + format_quoted_list(items, quote, "or") +} + +fn format_quoted_list(items: &[T], quote: char, conjunction: &str) -> String { + let quoted: Vec = items + .iter() + .map(|item| format!("{}{}{}", quote, item, quote)) + .collect(); + format_list("ed, conjunction) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_and_list_empty() { + let empty: &[&str] = &[]; + assert_eq!(and_list(empty), ""); + } + + #[test] + fn test_and_list_single() { + assert_eq!(and_list(&["apple"]), "apple"); + } + + #[test] + fn test_and_list_two() { + assert_eq!(and_list(&["apple", "banana"]), "apple and banana"); + } + + #[test] + fn test_and_list_three() { + assert_eq!( + and_list(&["apple", "banana", "cherry"]), + "apple, banana, and cherry" + ); + } + + #[test] + fn test_or_list_two() { + assert_eq!(or_list(&["apple", "banana"]), "apple or banana"); + } + + #[test] + fn test_or_list_three() { + assert_eq!( + or_list(&["apple", "banana", "cherry"]), + "apple, banana, or cherry" + ); + } + + #[test] + fn test_and_list_quoted_empty() { + let empty: &[&str] = &[]; + assert_eq!(and_list_quoted(empty, '\''), ""); + } + + #[test] + fn test_and_list_quoted_single() { + assert_eq!(and_list_quoted(&["apple"], '\''), "'apple'"); + } + + #[test] + fn test_and_list_quoted_two() { + assert_eq!( + and_list_quoted(&["apple", "banana"], '\''), + "'apple' and 'banana'" + ); + } + + #[test] + fn test_and_list_quoted_three() { + assert_eq!( + and_list_quoted(&["apple", "banana", "cherry"], '\''), + "'apple', 'banana', and 'cherry'" + ); + } + + #[test] + fn test_and_list_quoted_four() { + assert_eq!( + and_list_quoted(&["a", "b", "c", "d"], '\''), + "'a', 'b', 'c', and 'd'" + ); + } + + #[test] + fn test_or_list_quoted_two() { + assert_eq!( + or_list_quoted(&["apple", "banana"], '"'), + "\"apple\" or \"banana\"" + ); + } + + #[test] + fn test_or_list_quoted_three() { + assert_eq!( + or_list_quoted(&["apple", "banana", "cherry"], '`'), + "`apple`, `banana`, or `cherry`" + ); + } + + #[test] + fn test_with_string_vec() { + let items = vec!["one".to_string(), "two".to_string()]; + assert_eq!(and_list_quoted(&items, '\''), "'one' and 'two'"); + } + + // Enum tests + #[derive(Debug)] + enum Color { + Red, + Green, + Blue, + } + + impl std::fmt::Display for Color { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Color::Red => write!(f, "red"), + Color::Green => write!(f, "green"), + Color::Blue => write!(f, "blue"), + } + } + } + + #[test] + fn test_and_list_enum() { + assert_eq!( + and_list(&[Color::Red, Color::Green, Color::Blue]), + "red, green, and blue" + ); + } + + #[test] + fn test_or_list_enum() { + assert_eq!(or_list(&[Color::Red, Color::Green]), "red or green"); + } + + #[test] + fn test_and_list_quoted_enum() { + assert_eq!( + and_list_quoted(&[Color::Red, Color::Green, Color::Blue], '\''), + "'red', 'green', and 'blue'" + ); + } + + #[test] + fn test_or_list_quoted_enum() { + assert_eq!( + or_list_quoted(&[Color::Red, Color::Green], '"'), + "\"red\" or \"green\"" + ); + } + + #[test] + fn test_with_numbers() { + assert_eq!(and_list(&[1, 2, 3]), "1, 2, and 3"); + assert_eq!(or_list(&[42, 99]), "42 or 99"); + } +} diff --git a/src/writer/vegalite/layer.rs b/src/writer/vegalite/layer.rs index 3a82cf19..dcdc5092 100644 --- a/src/writer/vegalite/layer.rs +++ b/src/writer/vegalite/layer.rs @@ -76,7 +76,7 @@ pub fn validate_layer_columns(layer: &Layer, data: &DataFrame, layer_idx: usize) aesthetic, layer_idx + 1, source_desc, - available_columns.join(", ") + crate::and_list(&available_columns) ))); } } @@ -95,7 +95,7 @@ pub fn validate_layer_columns(layer: &Layer, data: &DataFrame, layer_idx: usize) col, layer_idx + 1, source_desc, - available_columns.join(", ") + crate::and_list(&available_columns) ))); } }