Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

warnings! #751

Merged
merged 3 commits into from
Feb 11, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions src/options.js
Original file line number Diff line number Diff line change
@@ -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";
Expand Down Expand Up @@ -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;
Expand Down
16 changes: 15 additions & 1 deletion src/plot.js
Original file line number Diff line number Diff line change
@@ -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";
Expand All @@ -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;
Expand Down Expand Up @@ -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("⚠️")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.text("⚠️")
.attr("cursor", "help")
.text("⚠️")

semantically this would feel more appropriate than the text cursor, but it occludes the ⚠️ symbol

.append("title")
.text(`${w.toLocaleString("en-US")} warning${w === 1 ? "" : "s"}. Please check the console.`);
}

return figure;
}

Expand Down
27 changes: 25 additions & 2 deletions src/scales.js
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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";
Expand Down Expand Up @@ -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}) {
Expand Down
9 changes: 6 additions & 3 deletions src/transforms/window.js
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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));
}
Expand All @@ -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";
Expand Down
12 changes: 12 additions & 0 deletions src/warnings.js
Original file line number Diff line number Diff line change
@@ -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;
}
Loading