warnings! (#751)

* warnings!

* uplift an old warning

* numeric string warning

Co-authored-by: Philippe Rivière <[email protected]>

src/options.js

@@ -1,3 +1,4 @@
+import {parse as isoParse} from "isoformat";
 import {color, descending} from "d3";
 import {symbolAsterisk, symbolDiamond2, symbolPlus, symbolSquare2, symbolTriangle2, symbolX as symbolTimes} from "d3";
 import {symbolCircle, symbolCross, symbolDiamond, symbolSquare, symbolStar, symbolTriangle, symbolWye} from "d3";
@@ -210,6 +211,26 @@ export function isTemporal(values) {
   }
 }
 
+// Are these strings that might represent dates? This is stricter than ISO 8601
+// because we want to ignore false positives on numbers; for example, the string
+// "1192" is more likely to represent a number than a date even though it is
+// valid ISO 8601 representing 1192-01-01.
+export function isTemporalString(values) {
+  for (const value of values) {
+    if (value == null) continue;
+    return typeof value === "string" && isNaN(value) && isoParse(value);
+  }
+}
+
+// Are these strings that might represent numbers? This is stricter than
+// coercion because we want to ignore false positives on e.g. empty strings.
+export function isNumericString(values) {
+  for (const value of values) {
+    if (value == null || value === "") continue;
+    return typeof value === "string" && !isNaN(value);
+  }
+}
+
 export function isNumeric(values) {
   for (const value of values) {
     if (value == null) continue;

src/plot.js

@@ -1,4 +1,4 @@
-import {create, cross, difference, groups, InternMap} from "d3";
+import {create, cross, difference, groups, InternMap, select} from "d3";
 import {Axes, autoAxisTicks, autoScaleLabels} from "./axes.js";
 import {Channel, channelSort} from "./channel.js";
 import {defined} from "./defined.js";
@@ -8,6 +8,7 @@ import {arrayify, isOptions, keyword, range, first, second, where} from "./optio
 import {Scales, ScaleFunctions, autoScaleRange, applyScales, exposeScales} from "./scales.js";
 import {applyInlineStyles, maybeClassName, maybeClip, styles} from "./style.js";
 import {basic} from "./transforms/basic.js";
+import {consumeWarnings} from "./warnings.js";
 
 export function plot(options = {}) {
   const {facet, style, caption, ariaLabel, ariaDescription} = options;
@@ -119,6 +120,19 @@ export function plot(options = {}) {
 
   figure.scale = exposeScales(scaleDescriptors);
   figure.legend = exposeLegends(scaleDescriptors, options);
+
+  const w = consumeWarnings();
+  if (w > 0) {
+    select(svg).append("text")
+        .attr("x", width)
+        .attr("y", 20)
+        .attr("dy", "-1em")
+        .attr("text-anchor", "end")
+        .text("⚠️")
+      .append("title")
+        .text(`${w.toLocaleString("en-US")} warning${w === 1 ? "" : "s"}. Please check the console.`);
+  }
+
   return figure;
 }
 

src/scales.js

@@ -1,10 +1,11 @@
 import {parse as isoParse} from "isoformat";
-import {isColor, isEvery, isOrdinal, isFirst, isSymbol, isTemporal, maybeSymbol, order} from "./options.js";
+import {isColor, isEvery, isOrdinal, isFirst, isSymbol, isTemporal, maybeSymbol, order, isTemporalString, isNumericString} from "./options.js";
 import {registry, color, position, radius, opacity, symbol, length} from "./scales/index.js";
 import {ScaleLinear, ScaleSqrt, ScalePow, ScaleLog, ScaleSymlog, ScaleQuantile, ScaleThreshold, ScaleIdentity} from "./scales/quantitative.js";
 import {ScaleDiverging, ScaleDivergingSqrt, ScaleDivergingPow, ScaleDivergingLog, ScaleDivergingSymlog} from "./scales/diverging.js";
 import {ScaleTime, ScaleUtc} from "./scales/temporal.js";
 import {ScaleOrdinal, ScalePoint, ScaleBand, ordinalImplicit} from "./scales/ordinal.js";
+import {warn} from "./warnings.js";
 
 export function Scales(channels, {
   inset: globalInset = 0,
@@ -133,6 +134,24 @@ export function normalizeScale(key, scale, hint) {
 
 function Scale(key, channels = [], options = {}) {
   const type = inferScaleType(key, channels, options);
+
+  // Warn for common misuses of implicit ordinal scales. We disable this test if
+  // you set the domain or range explicitly, since setting the domain or range
+  // (typically with a cardinality of more than two) is another indication that
+  // you intended for the scale to be ordinal; we also disable it for facet
+  // scales since these are always band scales.
+  if (options.type === undefined
+      && options.domain === undefined
+      && options.range === undefined
+      && key !== "fx"
+      && key !== "fy"
+      && isOrdinalScale({type})) {
+    const values = channels.map(({value}) => value).filter(value => value !== undefined);
+    if (values.some(isTemporal)) warn(`Warning: some data associated with the ${key} scale are dates. Dates are typically associated with a "utc" or "time" scale rather than a "${formatScaleType(type)}" scale. If you are using a bar mark, you probably want a rect mark with the interval option instead; if you are using a group transform, you probably want a bin transform instead. If you want to treat this data as ordinal, you can suppress this warning by setting the type of the ${key} scale to "${formatScaleType(type)}".`);
+    else if (values.some(isTemporalString)) warn(`Warning: some data associated with the ${key} scale are strings that appear to be dates (e.g., YYYY-MM-DD). If these strings represent dates, you should parse them to Date objects. Dates are typically associated with a "utc" or "time" scale rather than a "${formatScaleType(type)}" scale. If you are using a bar mark, you probably want a rect mark with the interval option instead; if you are using a group transform, you probably want a bin transform instead. If you want to treat this data as ordinal, you can suppress this warning by setting the type of the ${key} scale to "${formatScaleType(type)}".`);
+    else if (values.some(isNumericString)) warn(`Warning: some data associated with the ${key} scale are strings that appear to be numbers. If these strings represent numbers, you should parse or coerce them to numbers. Numbers are typically associated with a "linear" scale rather than a "${formatScaleType(type)}" scale. If you want to treat this data as ordinal, you can suppress this warning by setting the type of the ${key} scale to "${formatScaleType(type)}".`);
+  }
+
   options.type = type; // Mutates input!
 
   // Once the scale type is known, coerce the associated channel values and any
@@ -190,6 +209,10 @@ function Scale(key, channels = [], options = {}) {
   }
 }
 
+function formatScaleType(type) {
+  return typeof type === "symbol" ? type.description : type;
+}
+
 function inferScaleType(key, channels, {type, domain, range, scheme}) {
   // The facet scales are always band scales; this cannot be changed.
   if (key === "fx" || key === "fy") return "band";
@@ -272,7 +295,7 @@ export function isTemporalScale({type}) {
 }
 
 export function isOrdinalScale({type}) {
-  return type === "ordinal" || type === "point" || type === "band";
+  return type === "ordinal" || type === "point" || type === "band" || type === ordinalImplicit;
 }
 
 function isThresholdScale({type}) {

src/transforms/window.js

@@ -1,5 +1,6 @@
 import {mapX, mapY} from "./map.js";
 import {deviation, max, min, median, mode, variance} from "d3";
+import {warn} from "../warnings.js";
 
 export function windowX(windowOptions = {}, options) {
   if (arguments.length === 1) options = windowOptions;
@@ -13,7 +14,11 @@ export function windowY(windowOptions = {}, options) {
 
 export function window(options = {}) {
   if (typeof options === "number") options = {k: options};
-  let {k, reduce, shift, anchor = maybeShift(shift)} = options;
+  let {k, reduce, shift, anchor} = options;
+  if (anchor === undefined && shift !== undefined) {
+    anchor = maybeShift(shift);
+    warn(`Warning: the shift option is deprecated; please use anchor "${anchor}" instead.`);
+  }
   if (!((k = Math.floor(k)) > 0)) throw new Error("invalid k");
   return maybeReduce(reduce)(k, maybeAnchor(anchor, k));
 }
@@ -28,8 +33,6 @@ function maybeAnchor(anchor = "middle", k) {
 }
 
 function maybeShift(shift) {
-  if (shift === undefined) return;
-  console.warn("shift is deprecated; please use anchor instead");
   switch (`${shift}`.toLowerCase()) {
     case "centered": return "middle";
     case "leading": return "start";

src/warnings.js

@@ -0,0 +1,12 @@
+let warnings = 0;
+
+export function consumeWarnings() {
+  const w = warnings;
+  warnings = 0;
+  return w;
+}
+
+export function warn(message) {
+  console.warn(message);
+  ++warnings;
+}