From b9551dbf3c199d854c68dc1f687f88c6481f4b90 Mon Sep 17 00:00:00 2001 From: Brian Sperlongano Date: Fri, 23 Jun 2023 18:34:57 -0400 Subject: [PATCH 01/15] Add statistics check --- .github/workflows/build-preview.yml | 45 ++++- CONTRIBUTING.md | 14 +- icons/oneway.svg | 6 - icons/oneway_black.svg | 3 + icons/oneway_white.svg | 3 + package.json | 1 + scripts/extract_layer.js | 56 ++++++ scripts/stats.js | 121 ++++++------ scripts/stats_compare.js | 89 +++++++++ src/layer/index.js | 5 +- src/layer/oneway.js | 296 +++++----------------------- src/layer/transportation_label.js | 1 + test/test_stats_compare.sh | 51 +++++ 13 files changed, 371 insertions(+), 320 deletions(-) delete mode 100644 icons/oneway.svg create mode 100644 icons/oneway_black.svg create mode 100644 icons/oneway_white.svg create mode 100644 scripts/extract_layer.js create mode 100644 scripts/stats_compare.js create mode 100755 test/test_stats_compare.sh diff --git a/.github/workflows/build-preview.yml b/.github/workflows/build-preview.yml index f42569976..ce048cda7 100644 --- a/.github/workflows/build-preview.yml +++ b/.github/workflows/build-preview.yml @@ -12,12 +12,31 @@ permissions: pages: write pull-requests: write id-token: write + checks: write concurrency: preview-${{ github.ref }} jobs: deploy-preview: runs-on: ubuntu-latest steps: - - name: Checkout 🛎️ + - name: Checkout Main Branch 🛎️ + uses: actions/checkout@v3 + with: + ref: main + - name: Install and Build Main Branch 🔧 + run: | + npm ci --include=dev + npm run build + npm run style + npm run shields + cp src/configs/config.aws.js src/config.js + - name: Capture main branch usage statistics + id: main-stats + run: | + MAIN_STATS=$(node scripts/stats.js -j) + echo "MAIN_STATS<> $GITHUB_ENV + echo -e "$MAIN_STATS" >> $GITHUB_ENV + echo "EOF" >> $GITHUB_ENV + - name: Checkout PR Branch 🛎️ uses: actions/checkout@v3 - name: Use Node.js 18.x uses: actions/setup-node@v3 @@ -42,6 +61,30 @@ jobs: npm run style npm run shields cp src/configs/config.aws.js src/config.js + - name: Capture PR branch usage statistics + id: pr-stats + run: | + PR_STATS=$(node scripts/stats.js -j) + echo "PR_STATS<> $GITHUB_ENV + echo -e "$PR_STATS" >> $GITHUB_ENV + echo "EOF" >> $GITHUB_ENV + - name: Compare Stats + id: compare-stats + run: | + mkdir stats + echo '${{ env.MAIN_STATS }}' + echo '${{ env.PR_STATS }}' + node scripts/stats_compare '${{ env.MAIN_STATS }}' '${{ env.PR_STATS }}' > stats/stats-difference.md + - name: Print Stats to GitHub Checks + uses: LouisBrunner/checks-action@v1.6.1 + if: always() + with: + token: ${{ secrets.GITHUB_TOKEN }} + name: Performance Metrics + conclusion: neutral + output: | + {"summary":"Style size changes introduced by this PR"} + output_text_description_file: stats/stats-difference.md - name: Upload Build artifact uses: actions/upload-artifact@v3 with: diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ea878665e..62a22b609 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -190,13 +190,19 @@ When adding or changing style layer code, it can be helpful to assess the change There is a "stats" script that will generate various statistics about layer composition and complexity: -- `npm run stats -- -a -s` - overall size and breakdown of layers -- `npm run stats -- -c` - total layer count -- `npm run stats -- -h` - list all options +- `npm run -s stats -- -a -s` - overall size and breakdown of layers +- `npm run -s stats -- -c` - total layer count +- `npm run -s stats -- -h` - list all options + +There is an "extract_layers" script that will extract layer style data: + +- `npm run -s extract_layer -pl ` - JSON contents of a specified layer +- `npm run -s extract_layer -pg ` - list of layers from a specified source +- `npm run -s extract_layer -h` - list all options ## Layers -1. Layers should be named as followed: `_`, wher the "group" should match the file name that the layer is contained in. This naming convention is needed by the layer statistic script. +1. Layers must be uniquely named. 2. For performance reasons, it is better to have fewer layers with filters than multiple, simpler layers. 3. Layers are drawn in the order specified in `layer/index.js` using the [Painter's Algorithm](https://en.wikipedia.org/wiki/Painter%27s_algorithm). diff --git a/icons/oneway.svg b/icons/oneway.svg deleted file mode 100644 index 476bf8103..000000000 --- a/icons/oneway.svg +++ /dev/null @@ -1,6 +0,0 @@ - - - - - - diff --git a/icons/oneway_black.svg b/icons/oneway_black.svg new file mode 100644 index 000000000..825ab153d --- /dev/null +++ b/icons/oneway_black.svg @@ -0,0 +1,3 @@ + + + diff --git a/icons/oneway_white.svg b/icons/oneway_white.svg new file mode 100644 index 000000000..70dc2aae4 --- /dev/null +++ b/icons/oneway_white.svg @@ -0,0 +1,3 @@ + + + diff --git a/package.json b/package.json index d82733cd3..813895eec 100644 --- a/package.json +++ b/package.json @@ -23,6 +23,7 @@ "code_format": "run-s code_format:prettier code_format:svgo", "code_format:prettier": "prettier --write --list-different .", "code_format:svgo": "svgo -q -f icons/", + "extract_layer": "node scripts/extract_layer", "presprites": "shx rm -rf dist/sprites", "serve": "ts-node scripts/serve", "shields": "node scripts/generate_shield_defs.js -o dist/shields.json", diff --git a/scripts/extract_layer.js b/scripts/extract_layer.js new file mode 100644 index 000000000..e6eb4cc3b --- /dev/null +++ b/scripts/extract_layer.js @@ -0,0 +1,56 @@ +import * as Style from "../src/js/style.js"; +import config from "../src/config.js"; +import { Command, Option } from "commander"; + +const program = new Command(); +program + .option("-pp, --pretty", "pretty-print JSON output") + .addOption( + new Option( + "-pg, --print-group ", + "print a list of the layers in a group" + ).conflicts("printLayer") + ) + .option("-pl, --print-layer ", "print the JSON of a layer") + .option("-loc, --locales ", "language codes", ["mul"]); +program.parse(process.argv); + +const opts = program.opts(); + +if (Object.keys(opts).length === 1) program.help(); + +const locales = opts.locales[0].split(","); + +const style = Style.build( + config.OPENMAPTILES_URL, + "https://zelonewolf.github.io/openstreetmap-americana/sprites/sprite", + "https://osm-americana.github.io/fontstack66/{fontstack}/{range}.pbf", + locales +); + +const layers = style.layers; +const layerMap = new Map(); +const layerGroupMap = new Map(); + +for (let i = 0; i < layers.length; i++) { + const layer = layers[i]; + layerMap.set(layer.id, layers[i]); + const layerGroup = layer["source-layer"] || layer.source || layer.type; + if (!layerGroupMap.has(layerGroup)) { + layerGroupMap.set(layerGroup, [layer.id]); + } else { + layerGroupMap.get(layerGroup).push(layer.id); + } +} + +let outputObj; + +if (opts.printGroup) { + outputObj = layerGroupMap.get(opts.printGroup); +} + +if (opts.printLayer) { + outputObj = layerMap.get(opts.printLayer) ?? {}; +} + +process.stdout.write(JSON.stringify(outputObj, null, opts.pretty ? 2 : null)); diff --git a/scripts/stats.js b/scripts/stats.js index 26662d65e..b75ef7f01 100644 --- a/scripts/stats.js +++ b/scripts/stats.js @@ -1,93 +1,94 @@ import * as Style from "../src/js/style.js"; import config from "../src/config.js"; -import { Command } from "commander"; +import { Command, Option } from "commander"; const program = new Command(); program - .option("-a, --all-layers", "summary layer stats") - .option("-c, --layer-count", "count number of layers") - .option("-s, --layer-size", "size of all layers") - .option("-l, --layer ", "stats about one layer") - .option("-loc, --locales ", "language codes", ["mul"]) - .option("-pp, --pretty", "pretty-print JSON output") - .option( - "-pg, --print-group ", - "print a list of the layers in a group" + .addOption( + new Option("-a, --all-layers", "summary layer stats") + .conflicts("layerCount") + .conflicts("layerSize") + .conflicts("allJson") + ) + .addOption( + new Option("-c, --layer-count", "count number of layers") + .conflicts("layerSize") + .conflicts("allJson") ) - .option("-pl, --print-layer ", "print the JSON of a layer"); + .addOption( + new Option("-s, --layer-size", "size of all layers").conflicts("allJson") + ) + .option("-loc, --locales ", "language codes", ["mul"]) + .option("-j, --all-json", "output all stats in JSON") + .option("-pp, --pretty", "pretty-print JSON output"); + program.parse(process.argv); -let opts = program.opts(); +const opts = program.opts(); if (Object.keys(opts).length === 1) program.help(); -let style = Style.build( +const locales = opts.locales[0].split(","); + +const style = Style.build( config.OPENMAPTILES_URL, "https://zelonewolf.github.io/openstreetmap-americana/sprites/sprite", "https://osm-americana.github.io/fontstack66/{fontstack}/{range}.pbf", - opts.locales + locales ); const layers = style.layers; const layerCount = layers.length; -const layerSize = JSON.stringify(layers).length; + +if (opts.layerCount) { + console.log(layerCount); + process.exit(); +} + +const styleSize = JSON.stringify(layers).length; + +if (opts.layerSize) { + console.log(styleSize); + process.exit(); +} const layerMap = new Map(); -const layerGroupMap = new Map(); -const layerSizeStats = new Map(); -const layerGroupSizeStats = new Map(); -const layerGroupCountStats = new Map(); -for (let i = 0; i < layers.length; i++) { - let layer = layers[i]; +const stats = { + layerCount, + styleSize, + layerGroup: {}, +}; + +for (let i = 0; i < layerCount; i++) { + const layer = layers[i]; layerMap.set(layer.id, layers[i]); - let layerSize = JSON.stringify(layer).length; - let layerGroup = layer.id.split("_", 1)[0]; - layerSizeStats.set(layer.id, JSON.stringify(layer).length); - if (!layerGroupSizeStats.has(layerGroup)) { - layerGroupSizeStats.set(layerGroup, layerSize); - layerGroupCountStats.set(layerGroup, 1); - layerGroupMap.set(layerGroup, [layer.id]); + const layerSize = JSON.stringify(layer).length; + const layerGroup = layer["source-layer"] || layer.source || layer.type; + if (stats.layerGroup[layerGroup]) { + stats.layerGroup[layerGroup].size += layerSize; + stats.layerGroup[layerGroup].layerCount++; } else { - layerGroupSizeStats.set( - layerGroup, - layerGroupSizeStats.get(layerGroup) + layerSize - ); - layerGroupCountStats.set( - layerGroup, - layerGroupCountStats.get(layerGroup) + 1 - ); - layerGroupMap.get(layerGroup).push(layer.id); + stats.layerGroup[layerGroup] = { + size: layerSize, + layerCount: 1, + }; } } -if (opts.layerCount) { - console.log(`${layerCount} layers`); -} - -if (opts.layerSize) { - console.log(`Total layer size ${layerSize.toLocaleString("en-US")} bytes`); +if (opts.allJson) { + process.stdout.write(JSON.stringify(stats, null, opts.pretty ? 2 : null)); + process.exit(); } if (opts.allLayers) { - console.log(`${layerCount} layers, grouped as follows:`); - layerGroupSizeStats.forEach((v, k) => { - let layerCount = layerGroupCountStats.get(k); - let layerString = `${k}(${layerCount})`.padEnd(30, "."); + for (const layerGroup in stats.layerGroup) { + let layerStats = stats.layerGroup[layerGroup]; + let layerString = `${layerGroup}(${layerStats.layerCount})`.padEnd(30, "."); console.log( - `${layerString}${v.toLocaleString("en-US").padStart(10, ".")} bytes` + `${layerString}${layerStats.size + .toLocaleString("en-US") + .padStart(10, ".")} bytes` ); - }); -} - -if (opts.printGroup) { - layerGroupMap.get(opts.printGroup).forEach((lyr) => console.log(lyr)); -} - -if (opts.printLayer) { - if (opts.pretty) { - console.log(JSON.stringify(layerMap.get(opts.printLayer), null, 2)); - } else { - console.log(JSON.stringify(layerMap.get(opts.printLayer))); } } diff --git a/scripts/stats_compare.js b/scripts/stats_compare.js new file mode 100644 index 000000000..488ea9a6d --- /dev/null +++ b/scripts/stats_compare.js @@ -0,0 +1,89 @@ +function calculateDifference(object1, object2) { + const difference = {}; + + for (const key in object1) { + if (typeof object1[key] === "object") { + difference[key] = calculateDifference(object1[key], object2[key]); + } else if (typeof object1[key] === "number") { + difference[key] = object2[key] - object1[key]; + } + } + + return difference; +} + +const stats1 = JSON.parse(process.argv[2]); +const stats2 = JSON.parse(process.argv[3]); + +const difference = calculateDifference(stats1, stats2); + +const pctFormat = { + style: "percent", + minimumFractionDigits: 1, + maximumFractionDigits: 1, + signDisplay: "exceptZero", +}; + +const summaryChange = ` + +## Style size statistics + +| | main | this PR | change | % change | +|-----------|--------------:|-------------:|----------------:|----------------:| +| Layers | ${stats1.layerCount} | ${stats2.layerCount} | ${ + difference.layerCount +} | ${((100 * difference.layerCount) / stats1.layerCount).toLocaleString( + undefined, + pctFormat +)} | +| Size (b) | ${stats1.styleSize.toLocaleString( + "en" +)} | ${stats2.styleSize.toLocaleString( + "en" +)} | ${difference.styleSize.toLocaleString("en")} | ${( + (100 * difference.styleSize) / + stats1.styleSize +).toLocaleString(undefined, pctFormat)} + +`; + +let layerCountChange = ` + +## Layer count comparison + +| | main | this PR | change | % change | +|-----------|--------------:|-------------:|----------------:|----------------:| +`; + +for (const layer in difference.layerGroup) { + layerCountChange += `${layer} | ${stats1.layerGroup[layer].layerCount} | ${ + stats2.layerGroup[layer].layerCount + } | ${difference.layerGroup[layer].layerCount} | ${( + difference.layerGroup[layer].layerCount / + stats1.layerGroup[layer].layerCount + ).toLocaleString(undefined, pctFormat)} +`; +} + +let layerSizeChange = ` + +## Layer size comparison + +| | main | this PR | change | % change | +|-----------|--------------:|-------------:|----------------:|----------------:| +`; + +for (const layer in difference.layerGroup) { + layerSizeChange += `${layer} | ${stats1.layerGroup[layer].size.toLocaleString( + "en" + )} | ${stats2.layerGroup[layer].size.toLocaleString( + "en" + )} | ${difference.layerGroup[layer].size.toLocaleString("en")} | ${( + difference.layerGroup[layer].size / stats1.layerGroup[layer].size + ).toLocaleString(undefined, pctFormat)} +`; +} + +console.log(summaryChange); +console.log(layerCountChange); +console.log(layerSizeChange); diff --git a/src/layer/index.js b/src/layer/index.js index 0aa3d8ecd..95fada71d 100644 --- a/src/layer/index.js +++ b/src/layer/index.js @@ -78,7 +78,6 @@ export function build(locales) { lyrRoad.roadTunnel.fill(), lyrOneway.tunnel, - lyrOneway.tunnelLink, lyrFerry.ferry, @@ -136,8 +135,7 @@ export function build(locales) { lyrRail.railway.fill(), - lyrOneway.road, - lyrOneway.link, + lyrOneway.surface, lyrAerialway.dragLift ); @@ -197,7 +195,6 @@ export function build(locales) { lyrRail.railwayBridge.fill(), lyrOneway.bridge, - lyrOneway.bridgeLink, ]; layers.push(...lyrRail.getLayerSeparatedBridgeLayers(bridgeLayers)); diff --git a/src/layer/oneway.js b/src/layer/oneway.js index ea8438bf9..47253b9c6 100644 --- a/src/layer/oneway.js +++ b/src/layer/oneway.js @@ -1,285 +1,91 @@ "use strict"; -export const road = { - id: "road_oneway", - type: "symbol", - paint: { - "icon-opacity": 0.5, - }, +// Common expressions +const highwaySelector = ["match", ["get", "class"]]; + +export const surface = { + id: "oneway_surface", filter: [ "all", ["==", ["get", "oneway"], 1], - ["!", ["in", ["get", "brunnel"], ["literal", ["bridge", "tunnel"]]]], ["!=", ["get", "ramp"], 1], - [ - "in", - ["get", "class"], - [ - "literal", - [ - "motorway", - // "trunk", - // "primary", - // "secondary", - // "tertiary", - // "minor", - // "service", - ], - ], - ], + ["!", ["in", ["get", "brunnel"], ["literal", ["bridge", "tunnel"]]]], ], - layout: { - "icon-size": { - stops: [ - [15, 0.5], - [19, 1], - ], - }, - "icon-image": "oneway", - visibility: "visible", - "icon-padding": 2, - "symbol-spacing": 75, - "symbol-placement": "line", - "icon-rotation-alignment": "map", - }, source: "openmaptiles", - minzoom: 15, "source-layer": "transportation", -}; - -export const tunnel = { - id: "tunnel_oneway", type: "symbol", - paint: { - "icon-opacity": 0.5, - }, - filter: [ - "all", - ["==", ["get", "oneway"], 1], - ["==", ["get", "brunnel"], "tunnel"], - ["!=", ["get", "ramp"], 1], - [ - "in", - ["get", "class"], - [ - "literal", - [ - "motorway", - // "trunk", - // "primary", - // "secondary", - // "tertiary", - // "minor", - // "service", - ], - ], - ], - ], - layout: { - "icon-size": { - stops: [ - [15, 0.5], - [19, 1], - ], - }, - "icon-image": "oneway", - visibility: "visible", - //"icon-rotate": 90, - "icon-padding": 2, - "symbol-spacing": 75, - "symbol-placement": "line", - "icon-rotation-alignment": "map", - }, - source: "openmaptiles", minzoom: 15, - "source-layer": "transportation", -}; -export const bridge = { - id: "bridge_oneway", - type: "symbol", - paint: { - "icon-opacity": 0.5, - }, - filter: [ - "all", - ["==", ["get", "oneway"], 1], - ["==", ["get", "brunnel"], "bridge"], - ["!=", ["get", "ramp"], 1], - [ - "in", - ["get", "class"], + layout: { + "icon-size": [ + "interpolate", + ["exponential", 1.2], + ["zoom"], + 15, + [...highwaySelector, ["motorway", "trunk", "primary"], 0.5, 0.3], + 19, [ - "literal", - [ - "motorway", - // "trunk", - // "primary", - // "secondary", - // "tertiary", - // "minor", - // "service", - ], + ...highwaySelector, + ["motorway", "trunk", "primary"], + 1, + "secondary", + 0.8, + 0.6, ], ], - ], - layout: { - "icon-size": { - stops: [ - [15, 0.5], - [19, 1], - ], - }, - "icon-image": "oneway", - visibility: "visible", - "icon-padding": 2, - "symbol-spacing": 75, - "symbol-placement": "line", - "icon-rotation-alignment": "map", - }, - source: "openmaptiles", - minzoom: 15, - "source-layer": "transportation", -}; - -export const link = { - id: "road_oneway_link", - type: "symbol", - paint: { - "icon-opacity": 0.5, - }, - filter: [ - "all", - ["==", ["get", "oneway"], 1], - ["!", ["in", ["get", "brunnel"], ["literal", ["bridge", "tunnel"]]]], - ["==", ["get", "ramp"], 1], - [ - "in", - ["get", "class"], + "icon-image": [ + "match", + ["get", "brunnel"], + "tunnel", + "oneway_black", [ - "literal", - [ - "motorway", - // "trunk", - // "primary", - // "secondary", - // "tertiary", - // "minor", - // "service", - ], + "match", + ["get", "toll"], + 1, + "oneway_black", + [...highwaySelector, "motorway", "oneway_white", "oneway_black"], ], ], - ], - layout: { - "icon-size": { - stops: [ - [15, 0.3], - [19, 0.8], - ], - }, - "icon-image": "oneway", visibility: "visible", "icon-padding": 2, - "symbol-spacing": 75, + "symbol-spacing": [ + "interpolate", + ["exponential", 1.2], + ["zoom"], + 15, + 75, + 19, + 300, + ], "symbol-placement": "line", "icon-rotation-alignment": "map", }, - source: "openmaptiles", - minzoom: 16, - "source-layer": "transportation", -}; - -export const tunnelLink = { - id: "tunnel_oneway_link", - type: "symbol", paint: { "icon-opacity": 0.5, }, +}; + +export const tunnel = { + ...surface, + id: "oneway_tunnel", filter: [ "all", ["==", ["get", "oneway"], 1], + ["!=", ["get", "ramp"], 1], ["==", ["get", "brunnel"], "tunnel"], - ["==", ["get", "ramp"], 1], - [ - "in", - ["get", "class"], - [ - "literal", - [ - "motorway", - // "trunk", - // "primary", - // "secondary", - // "tertiary", - // "minor", - // "service", - ], - ], - ], ], - layout: { - "icon-size": { - stops: [ - [15, 0.3], - [19, 0.8], - ], - }, - "icon-image": "oneway", - visibility: "visible", - //"icon-rotate": 90, - "icon-padding": 2, - "symbol-spacing": 75, - "symbol-placement": "line", - "icon-rotation-alignment": "map", + paint: { + "icon-opacity": 0.2, }, - source: "openmaptiles", - minzoom: 16, - "source-layer": "transportation", }; -export const bridgeLink = { - id: "bridge_oneway_link", - type: "symbol", - paint: { - "icon-opacity": 0.5, - }, +export const bridge = { + ...surface, + id: "oneway_bridge", filter: [ "all", ["==", ["get", "oneway"], 1], + ["!=", ["get", "ramp"], 1], ["==", ["get", "brunnel"], "bridge"], - ["==", ["get", "ramp"], 1], - [ - "in", - ["get", "class"], - [ - "literal", - [ - "motorway", - // "trunk", - // "primary", - // "secondary", - // "tertiary", - // "minor", - // "service", - ], - ], - ], ], - layout: { - "icon-size": { - stops: [ - [15, 0.3], - [19, 0.8], - ], - }, - "icon-image": "oneway", - visibility: "visible", - "icon-padding": 2, - "symbol-spacing": 75, - "symbol-placement": "line", - "icon-rotation-alignment": "map", - }, - source: "openmaptiles", - minzoom: 16, - "source-layer": "transportation", }; diff --git a/src/layer/transportation_label.js b/src/layer/transportation_label.js index 5ea963582..1da367753 100644 --- a/src/layer/transportation_label.js +++ b/src/layer/transportation_label.js @@ -176,6 +176,7 @@ export const bridgeSpacer = { "all", ["==", ["get", "brunnel"], "bridge"], ["in", ["geometry-type"], ["literal", ["LineString"]]], + ["!=", ["get", "oneway"], 1], ], paint: { "icon-opacity": 0, diff --git a/test/test_stats_compare.sh b/test/test_stats_compare.sh new file mode 100755 index 000000000..bfea3b6de --- /dev/null +++ b/test/test_stats_compare.sh @@ -0,0 +1,51 @@ +#!/bin/sh +DATA1=$(cat <<-EOM +{ + "layerCount": 349, + "styleSize": 1005817, + "layerGroup": { + "background": { "size": 122, "count": 1 }, + "landuse": { "size": 309, "count": 1 }, + "park": { "size": 2365, "count": 3 }, + "aeroway": { "size": 2059, "count": 7 }, + "landcover": { "size": 361, "count": 2 }, + "boundary": { "size": 13691, "count": 9 }, + "water": { "size": 912, "count": 3 }, + "waterway": { "size": 3543, "count": 3 }, + "transportation": { "size": 920221, "count": 299 }, + "building": { "size": 292, "count": 1 }, + "transportation_name": { "size": 8838, "count": 3 }, + "poi": { "size": 10094, "count": 2 }, + "water_name": { "size": 4647, "count": 2 }, + "aerodrome_label": { "size": 5951, "count": 4 }, + "place": { "size": 32062, "count": 9 } + } +} +EOM +) +DATA2=$(cat <<-EOM +{ + "layerCount": 351, + "styleSize": 1004717, + "layerGroup": { + "background": { "size": 122, "count": 1 }, + "landuse": { "size": 309, "count": 1 }, + "park": { "size": 2365, "count": 3 }, + "aeroway": { "size": 2059, "count": 7 }, + "landcover": { "size": 361, "count": 2 }, + "boundary": { "size": 13991, "count": 9 }, + "water": { "size": 901, "count": 1 }, + "waterway": { "size": 3743, "count": 5 }, + "transportation": { "size": 920221, "count": 299 }, + "building": { "size": 292, "count": 1 }, + "transportation_name": { "size": 8838, "count": 3 }, + "poi": { "size": 10094, "count": 2 }, + "water_name": { "size": 4647, "count": 2 }, + "aerodrome_label": { "size": 5951, "count": 4 }, + "place": { "size": 32062, "count": 9 } + } +} +EOM +) + +node scripts/stats_compare.js "$DATA1" "$DATA2" From f7cf152a61a21abcf41d33fa557f21312ecd7c00 Mon Sep 17 00:00:00 2001 From: Brian Sperlongano Date: Fri, 23 Jun 2023 18:43:46 -0400 Subject: [PATCH 02/15] Generate PR preview --- .github/workflows/build-preview.yml | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/.github/workflows/build-preview.yml b/.github/workflows/build-preview.yml index ce048cda7..b4b00c59c 100644 --- a/.github/workflows/build-preview.yml +++ b/.github/workflows/build-preview.yml @@ -90,3 +90,27 @@ jobs: with: name: americana path: dist/ + - name: Generate Preview text + run: | + echo "## PR Preview: + * [Map](https://preview.ourmap.us/pr/${{ env.PR_NUM }}/) + * [Shield Test](https://preview.ourmap.us/pr/${{ env.PR_NUM }}/shieldtest.html) + * [style.json](https://preview.ourmap.us/pr/${{ env.PR_NUM }}/style.json) + * [shields.json](https://preview.ourmap.us/pr/${{ env.PR_NUM }}/shields.json) + * [taginfo.json](https://preview.ourmap.us/pr/${{ env.PR_NUM }}/taginfo.json) + + ## Sprite Sheets: + + ![Sprites 1x](https://preview.ourmap.us/pr/${{ env.PR_NUM }}/sprites/sprite.png) + ![Sprites 2x](https://preview.ourmap.us/pr/${{ env.PR_NUM }}/sprites/sprite@2x.png) + " > pr_preview.md + - name: Print Preview Links to GitHub Checks + uses: LouisBrunner/checks-action@v1.6.1 + if: always() + with: + token: ${{ secrets.GITHUB_TOKEN }} + name: PR Preview + conclusion: neutral + output: | + {"summary":"Preview map changes introduced by this PR"} + output_text_description_file: pr_preview.md From c653d1eec0d7b64e729723b0d2277be9f257cc7b Mon Sep 17 00:00:00 2001 From: Brian Sperlongano Date: Fri, 23 Jun 2023 18:49:00 -0400 Subject: [PATCH 03/15] Move preview to tab --- .github/workflows/build-preview.yml | 4 ++-- .github/workflows/s3-upload.yml | 17 ----------------- 2 files changed, 2 insertions(+), 19 deletions(-) diff --git a/.github/workflows/build-preview.yml b/.github/workflows/build-preview.yml index b4b00c59c..3d6784c57 100644 --- a/.github/workflows/build-preview.yml +++ b/.github/workflows/build-preview.yml @@ -101,8 +101,8 @@ jobs: ## Sprite Sheets: - ![Sprites 1x](https://preview.ourmap.us/pr/${{ env.PR_NUM }}/sprites/sprite.png) - ![Sprites 2x](https://preview.ourmap.us/pr/${{ env.PR_NUM }}/sprites/sprite@2x.png) + * [Sprites 1x](https://preview.ourmap.us/pr/${{ env.PR_NUM }}/sprites/sprite.png) + * [Sprites 2x](https://preview.ourmap.us/pr/${{ env.PR_NUM }}/sprites/sprite@2x.png) " > pr_preview.md - name: Print Preview Links to GitHub Checks uses: LouisBrunner/checks-action@v1.6.1 diff --git a/.github/workflows/s3-upload.yml b/.github/workflows/s3-upload.yml index 351cbc58e..81dc0df57 100644 --- a/.github/workflows/s3-upload.yml +++ b/.github/workflows/s3-upload.yml @@ -72,20 +72,3 @@ jobs: AWS_REGION: ${{ secrets.AWS_REGION }} SOURCE_DIR: "./dist" DEST_DIR: pr/${{ env.PR_NUM }} - - name: Comment Pull Request - uses: thollander/actions-comment-pull-request@v2.3.1 - with: - message: | - PR Preview: - * [Map](https://preview.ourmap.us/pr/${{ env.PR_NUM }}/) - * [Shield Test](https://preview.ourmap.us/pr/${{ env.PR_NUM }}/shieldtest.html) - * [style.json](https://preview.ourmap.us/pr/${{ env.PR_NUM }}/style.json) - * [shields.json](https://preview.ourmap.us/pr/${{ env.PR_NUM }}/shields.json) - * [taginfo.json](https://preview.ourmap.us/pr/${{ env.PR_NUM }}/taginfo.json) - - Sprite Sheets: - - ![Sprites 1x](https://preview.ourmap.us/pr/${{ env.PR_NUM }}/sprites/sprite.png) - ![Sprites 2x](https://preview.ourmap.us/pr/${{ env.PR_NUM }}/sprites/sprite@2x.png) - comment_tag: pr_preview - pr_number: ${{ env.PR_NUM }} From c73f44619f2e2f737ddc4fd4fb66126906d2c5e0 Mon Sep 17 00:00:00 2001 From: Brian Sperlongano Date: Fri, 23 Jun 2023 18:50:34 -0400 Subject: [PATCH 04/15] Convert to img tags --- .github/workflows/build-preview.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/build-preview.yml b/.github/workflows/build-preview.yml index 3d6784c57..ad6421f18 100644 --- a/.github/workflows/build-preview.yml +++ b/.github/workflows/build-preview.yml @@ -101,8 +101,8 @@ jobs: ## Sprite Sheets: - * [Sprites 1x](https://preview.ourmap.us/pr/${{ env.PR_NUM }}/sprites/sprite.png) - * [Sprites 2x](https://preview.ourmap.us/pr/${{ env.PR_NUM }}/sprites/sprite@2x.png) + + " > pr_preview.md - name: Print Preview Links to GitHub Checks uses: LouisBrunner/checks-action@v1.6.1 From 735ddc661413c1ad73edbee4d5bf34092be08e8a Mon Sep 17 00:00:00 2001 From: Brian Sperlongano Date: Fri, 23 Jun 2023 19:00:34 -0400 Subject: [PATCH 05/15] Wait for image upload --- .github/workflows/build-preview.yml | 24 ------------ .github/workflows/pr-preview-check.yml | 53 ++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 24 deletions(-) create mode 100644 .github/workflows/pr-preview-check.yml diff --git a/.github/workflows/build-preview.yml b/.github/workflows/build-preview.yml index ad6421f18..ce048cda7 100644 --- a/.github/workflows/build-preview.yml +++ b/.github/workflows/build-preview.yml @@ -90,27 +90,3 @@ jobs: with: name: americana path: dist/ - - name: Generate Preview text - run: | - echo "## PR Preview: - * [Map](https://preview.ourmap.us/pr/${{ env.PR_NUM }}/) - * [Shield Test](https://preview.ourmap.us/pr/${{ env.PR_NUM }}/shieldtest.html) - * [style.json](https://preview.ourmap.us/pr/${{ env.PR_NUM }}/style.json) - * [shields.json](https://preview.ourmap.us/pr/${{ env.PR_NUM }}/shields.json) - * [taginfo.json](https://preview.ourmap.us/pr/${{ env.PR_NUM }}/taginfo.json) - - ## Sprite Sheets: - - - - " > pr_preview.md - - name: Print Preview Links to GitHub Checks - uses: LouisBrunner/checks-action@v1.6.1 - if: always() - with: - token: ${{ secrets.GITHUB_TOKEN }} - name: PR Preview - conclusion: neutral - output: | - {"summary":"Preview map changes introduced by this PR"} - output_text_description_file: pr_preview.md diff --git a/.github/workflows/pr-preview-check.yml b/.github/workflows/pr-preview-check.yml new file mode 100644 index 000000000..4577db136 --- /dev/null +++ b/.github/workflows/pr-preview-check.yml @@ -0,0 +1,53 @@ +name: Generate PR Preview Check +on: + pull_request: + branches: [main] + types: + - opened + - reopened + - synchronize + workflow_dispatch: +permissions: + checks: write +jobs: + deploy-preview: + runs-on: ubuntu-latest + steps: + - name: Wait for PR Preview Upload (1x Sprite) + uses: cygnetdigital/wait_for_response@v2.0.0 + with: + url: "https://preview.ourmap.us/pr/${{ github.event.pull_request.number }}/sprites/sprite.png" + responseCode: "200" + timeout: 120000 + interval: 500 + - name: Wait for PR Preview Upload (2x Sprite) + uses: cygnetdigital/wait_for_response@v2.0.0 + with: + url: "https://preview.ourmap.us/pr/${{ github.event.pull_request.number }}/sprites/sprite@2x.png" + responseCode: "200" + timeout: 120000 + interval: 500 + - name: Generate Preview text + run: | + echo "## PR Preview: + * [Map](https://preview.ourmap.us/pr/${{ github.event.pull_request.number }}/) + * [Shield Test](https://preview.ourmap.us/pr/${{ github.event.pull_request.number }}/shieldtest.html) + * [style.json](https://preview.ourmap.us/pr/${{ github.event.pull_request.number }}/style.json) + * [shields.json](https://preview.ourmap.us/pr/${{ github.event.pull_request.number }}/shields.json) + * [taginfo.json](https://preview.ourmap.us/pr/${{ github.event.pull_request.number }}/taginfo.json) + + ## Sprite Sheets: + + + + " > pr_preview.md + - name: Print Preview Links to GitHub Checks + uses: LouisBrunner/checks-action@v1.6.1 + if: always() + with: + token: ${{ secrets.GITHUB_TOKEN }} + name: PR Preview + conclusion: neutral + output: | + {"summary":"Preview map changes introduced by this PR"} + output_text_description_file: pr_preview.md From 9e97c420de9f07ff08174135ae0f42424ad53969 Mon Sep 17 00:00:00 2001 From: Brian Sperlongano Date: Sun, 9 Jul 2023 10:42:54 -0400 Subject: [PATCH 06/15] Remove old dev artifact --- test/test_stats_compare.sh | 51 -------------------------------------- 1 file changed, 51 deletions(-) delete mode 100755 test/test_stats_compare.sh diff --git a/test/test_stats_compare.sh b/test/test_stats_compare.sh deleted file mode 100755 index bfea3b6de..000000000 --- a/test/test_stats_compare.sh +++ /dev/null @@ -1,51 +0,0 @@ -#!/bin/sh -DATA1=$(cat <<-EOM -{ - "layerCount": 349, - "styleSize": 1005817, - "layerGroup": { - "background": { "size": 122, "count": 1 }, - "landuse": { "size": 309, "count": 1 }, - "park": { "size": 2365, "count": 3 }, - "aeroway": { "size": 2059, "count": 7 }, - "landcover": { "size": 361, "count": 2 }, - "boundary": { "size": 13691, "count": 9 }, - "water": { "size": 912, "count": 3 }, - "waterway": { "size": 3543, "count": 3 }, - "transportation": { "size": 920221, "count": 299 }, - "building": { "size": 292, "count": 1 }, - "transportation_name": { "size": 8838, "count": 3 }, - "poi": { "size": 10094, "count": 2 }, - "water_name": { "size": 4647, "count": 2 }, - "aerodrome_label": { "size": 5951, "count": 4 }, - "place": { "size": 32062, "count": 9 } - } -} -EOM -) -DATA2=$(cat <<-EOM -{ - "layerCount": 351, - "styleSize": 1004717, - "layerGroup": { - "background": { "size": 122, "count": 1 }, - "landuse": { "size": 309, "count": 1 }, - "park": { "size": 2365, "count": 3 }, - "aeroway": { "size": 2059, "count": 7 }, - "landcover": { "size": 361, "count": 2 }, - "boundary": { "size": 13991, "count": 9 }, - "water": { "size": 901, "count": 1 }, - "waterway": { "size": 3743, "count": 5 }, - "transportation": { "size": 920221, "count": 299 }, - "building": { "size": 292, "count": 1 }, - "transportation_name": { "size": 8838, "count": 3 }, - "poi": { "size": 10094, "count": 2 }, - "water_name": { "size": 4647, "count": 2 }, - "aerodrome_label": { "size": 5951, "count": 4 }, - "place": { "size": 32062, "count": 9 } - } -} -EOM -) - -node scripts/stats_compare.js "$DATA1" "$DATA2" From def25864d8d185eeeb24daf4b5124f25d8b78f78 Mon Sep 17 00:00:00 2001 From: Brian Sperlongano Date: Wed, 19 Jul 2023 21:12:02 -0400 Subject: [PATCH 07/15] Improve compare function and add test cases --- scripts/object_compare.ts | 72 ++++++++++++++++++++++++ scripts/stats_compare.js | 14 +---- test/stats_compare/stats_compare.spec.ts | 41 ++++++++++++++ 3 files changed, 114 insertions(+), 13 deletions(-) create mode 100644 scripts/object_compare.ts create mode 100644 test/stats_compare/stats_compare.spec.ts diff --git a/scripts/object_compare.ts b/scripts/object_compare.ts new file mode 100644 index 000000000..16587681a --- /dev/null +++ b/scripts/object_compare.ts @@ -0,0 +1,72 @@ +/** + * Calculates the difference between two objects by comparing their properties. + * If a property value is an object, it recursively calls itself to calculate the difference. + * Positive numbers for a property means that property is greater in object2 than object1. + * @param {Object|null} object1 - The first object to compare. + * @param {Object|null} object2 - The second object to compare. + * @returns {Object} - An object containing the differences between object2 and object1. + */ +export function calculateDifference( + object1: object | null, + object2: object | null +): object { + // If one object exists and the other doesn't, return the difference + if (object1 === null && object2 !== null) { + return object2; + } else if (object2 === null && object1 !== null) { + return negate(object1); + } + + const difference = {}; + + // Iterate through each property in object1 + for (const key in object1) { + if (typeof object1[key] === "object" && typeof object2![key] === "object") { + // Recursively calculate the difference for nested objects + difference[key] = calculateDifference(object1[key], object2![key]); + } else if ( + typeof object1[key] === "number" && + typeof object2![key] === "number" + ) { + // Calculate the difference for numeric properties + difference[key] = object2![key] - object1[key]; + } else { + // If the property exists in object1 but not in object2, include it in the result + difference[key] = negate(object1![key]); + } + } + + // Include properties that exist in object2 but not in object1 + for (const key in object2!) { + if (!(key in object1!)) { + difference[key] = object2[key]; + } + } + + return difference; +} + +/** + * Negate all numeric properties of this object. + * @param {Object} object - The object to process. + */ +function negate(object: object) { + if (typeof object === "number") { + return -object; + } + + // Create a new object to store the result + const result = {}; + + for (const key in object) { + if (typeof object[key] === "object" && object[key] !== null) { + // If the property value is an object (and not null), recursively process it + result[key] = negate(object[key]); + } else if (typeof object[key] === "number") { + // If the property value is a number, multiply it by the multiplier + result[key] = -object[key]; + } + } + + return result; +} diff --git a/scripts/stats_compare.js b/scripts/stats_compare.js index 488ea9a6d..8a6bbb076 100644 --- a/scripts/stats_compare.js +++ b/scripts/stats_compare.js @@ -1,16 +1,4 @@ -function calculateDifference(object1, object2) { - const difference = {}; - - for (const key in object1) { - if (typeof object1[key] === "object") { - difference[key] = calculateDifference(object1[key], object2[key]); - } else if (typeof object1[key] === "number") { - difference[key] = object2[key] - object1[key]; - } - } - - return difference; -} +import { calculateDifference } from "./object_compare"; const stats1 = JSON.parse(process.argv[2]); const stats2 = JSON.parse(process.argv[3]); diff --git a/test/stats_compare/stats_compare.spec.ts b/test/stats_compare/stats_compare.spec.ts new file mode 100644 index 000000000..932801176 --- /dev/null +++ b/test/stats_compare/stats_compare.spec.ts @@ -0,0 +1,41 @@ +import { calculateDifference } from "../../scripts/object_compare"; +import { expect } from "chai"; + +const a = 3; +const b = 4; + +const simpleA = { a }; +const simpleB = { b }; + +const complexA = { foo: { a } }; +const complexB = { bar: { b } }; + +const simpleA0 = { a: 0 }; +const complexA0 = { foo: { a: 0 } }; + +const negSimpleA = { a: -a }; +const negComplexA = { foo: { a: -a } }; + +const diffSimpleAB = { a: -a, b }; +const diffComplexAB = { foo: { a: -a }, bar: { b } }; + +describe("stats_compare", function () { + it("tests stats equality", function () { + expect(calculateDifference(simpleA, simpleA)).to.deep.equal(simpleA0); + expect(calculateDifference(complexA, complexA)).to.deep.equal(complexA0); + }); + it("tests stats remove", function () { + expect(calculateDifference(simpleA, null)).to.deep.equal(negSimpleA); + expect(calculateDifference(complexA, null)).to.deep.equal(negComplexA); + }); + it("tests stats add", function () { + expect(calculateDifference(null, simpleA)).to.deep.equal(simpleA); + expect(calculateDifference(null, complexA)).to.deep.equal(complexA); + }); + it("tests stats diff", function () { + expect(calculateDifference(simpleA, simpleB)).to.deep.equal(diffSimpleAB); + expect(calculateDifference(complexA, complexB)).to.deep.equal( + diffComplexAB + ); + }); +}); From 51dc895d08fd1a577da9b5f55c1a883e10580b38 Mon Sep 17 00:00:00 2001 From: Brian Sperlongano Date: Wed, 19 Jul 2023 21:59:35 -0400 Subject: [PATCH 08/15] Improve readability of stats generator --- .github/workflows/build-preview.yml | 2 +- scripts/stats_compare.js | 144 +++++++++++++++++++--------- 2 files changed, 102 insertions(+), 44 deletions(-) diff --git a/.github/workflows/build-preview.yml b/.github/workflows/build-preview.yml index ce048cda7..e0d0a56c7 100644 --- a/.github/workflows/build-preview.yml +++ b/.github/workflows/build-preview.yml @@ -74,7 +74,7 @@ jobs: mkdir stats echo '${{ env.MAIN_STATS }}' echo '${{ env.PR_STATS }}' - node scripts/stats_compare '${{ env.MAIN_STATS }}' '${{ env.PR_STATS }}' > stats/stats-difference.md + npm exec ts-node scripts/stats_compare '${{ env.MAIN_STATS }}' '${{ env.PR_STATS }}' > stats/stats-difference.md - name: Print Stats to GitHub Checks uses: LouisBrunner/checks-action@v1.6.1 if: always() diff --git a/scripts/stats_compare.js b/scripts/stats_compare.js index 8a6bbb076..458eebe4b 100644 --- a/scripts/stats_compare.js +++ b/scripts/stats_compare.js @@ -5,6 +5,15 @@ const stats2 = JSON.parse(process.argv[3]); const difference = calculateDifference(stats1, stats2); +const diffHeaderRow = [ + "| | main | this PR | change | % change |", + "|-----------|--------------:|-------------:|----------------:|----------------:|", +]; + +/** + * Show comparison of overall aggregate statistics between this PR and previous + */ + const pctFormat = { style: "percent", minimumFractionDigits: 1, @@ -12,66 +21,115 @@ const pctFormat = { signDisplay: "exceptZero", }; +const layersRow = [ + "Layers", + stats1.layerCount, + stats2.layerCount, + difference.layerCount, + ((100 * difference.layerCount) / stats1.layerCount).toLocaleString( + "en", + pctFormat + ), +]; + +const sizeRow = [ + "Size (b)", + stats1.styleSize.toLocaleString("en"), + stats2.styleSize.toLocaleString("en"), + difference.styleSize.toLocaleString("en"), + ((100 * difference.styleSize) / stats1.styleSize).toLocaleString( + "en", + pctFormat + ), +]; + +const summaryChangeTable = [ + ...diffHeaderRow, + ...layersRow.map((row) => row.join(" | ")), + ...sizeRow.map((row) => row.join(" | ")), +].join("\n"); + const summaryChange = ` ## Style size statistics -| | main | this PR | change | % change | -|-----------|--------------:|-------------:|----------------:|----------------:| -| Layers | ${stats1.layerCount} | ${stats2.layerCount} | ${ - difference.layerCount -} | ${((100 * difference.layerCount) / stats1.layerCount).toLocaleString( - undefined, - pctFormat -)} | -| Size (b) | ${stats1.styleSize.toLocaleString( - "en" -)} | ${stats2.styleSize.toLocaleString( - "en" -)} | ${difference.styleSize.toLocaleString("en")} | ${( - (100 * difference.styleSize) / - stats1.styleSize -).toLocaleString(undefined, pctFormat)} - +${summaryChangeTable} `; -let layerCountChange = ` +console.log(summaryChange); -## Layer count comparison +/** + * Show comparison of the number of layers in each group before and after + */ -| | main | this PR | change | % change | -|-----------|--------------:|-------------:|----------------:|----------------:| -`; +const layerCountChangeRows = []; for (const layer in difference.layerGroup) { - layerCountChange += `${layer} | ${stats1.layerGroup[layer].layerCount} | ${ - stats2.layerGroup[layer].layerCount - } | ${difference.layerGroup[layer].layerCount} | ${( - difference.layerGroup[layer].layerCount / - stats1.layerGroup[layer].layerCount - ).toLocaleString(undefined, pctFormat)} -`; + const differenceLayerCount = difference.layerGroup[layer].layerCount; + const percentageChange = differenceLayerCount / stats1LayerCount; + + const row = [ + layer, + stats1.layerGroup[layer]?.layerCount || 0, + stats2.layerGroup[layer]?.layerCount || 0, + differenceLayerCount.toLocaleString("en"), + percentageChange.toLocaleString("en", pctFormat), + ]; + + layerCountChangeRows.push(row); } -let layerSizeChange = ` +layerCountChange += layerCountChangeRows.join("\n"); -## Layer size comparison +const layerCountChangeTable = [ + ...diffHeaderRow, + ...layerCountChangeRows.map((row) => row.join(" | ")), +].join("\n"); + +const layerCountChange = ` -| | main | this PR | change | % change | -|-----------|--------------:|-------------:|----------------:|----------------:| +## Layer count comparison + +${layerCountChangeTable} `; +console.log(layerCountChange); + +/** + * Show comparison of the aggregate size of layers in each group before and after + */ + +const layerSizeChangeRows = []; + for (const layer in difference.layerGroup) { - layerSizeChange += `${layer} | ${stats1.layerGroup[layer].size.toLocaleString( - "en" - )} | ${stats2.layerGroup[layer].size.toLocaleString( - "en" - )} | ${difference.layerGroup[layer].size.toLocaleString("en")} | ${( - difference.layerGroup[layer].size / stats1.layerGroup[layer].size - ).toLocaleString(undefined, pctFormat)} -`; + const stats1LayerSize = stats1.layerGroup[layer]?.size || 0; + const stats2LayerSize = stats2.layerGroup[layer]?.size || 0; + const differenceLayerSize = difference.layerGroup[layer].size; + + const percentageChange = + stats1LayerSize !== 0 ? differenceLayerSize / stats1LayerSize : 0; + + const row = [ + layer, + stats1LayerSize.toLocaleString("en"), + stats2LayerSize.toLocaleString("en"), + differenceLayerSize.toLocaleString("en"), + percentageChange.toLocaleString("en", pctFormat), + ]; + + layerSizeChangeRows.push(row); } -console.log(summaryChange); -console.log(layerCountChange); +const layerSizeChangeTable = [ + ...diffHeaderRow, + ...layerSizeChangeRows.map((row) => row.join(" | ")), +].join("\n"); + +const layerSizeChange = ` + +## Layer size comparison + +${layerSizeChangeTable} +`; + console.log(layerSizeChange); From 88525a48c567d7647b76711c838ea5586f697079 Mon Sep 17 00:00:00 2001 From: Brian Sperlongano Date: Wed, 19 Jul 2023 22:04:24 -0400 Subject: [PATCH 09/15] Fix row join --- scripts/stats_compare.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/stats_compare.js b/scripts/stats_compare.js index 458eebe4b..9e8d7a9aa 100644 --- a/scripts/stats_compare.js +++ b/scripts/stats_compare.js @@ -45,8 +45,8 @@ const sizeRow = [ const summaryChangeTable = [ ...diffHeaderRow, - ...layersRow.map((row) => row.join(" | ")), - ...sizeRow.map((row) => row.join(" | ")), + layersRow.join(" | "), + sizeRow.join(" | "), ].join("\n"); const summaryChange = ` From 7d2bb698827725a544da2b6f5e0cfe2f13a66143 Mon Sep 17 00:00:00 2001 From: Brian Sperlongano Date: Wed, 19 Jul 2023 22:08:11 -0400 Subject: [PATCH 10/15] fix NPE bug --- scripts/stats_compare.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/scripts/stats_compare.js b/scripts/stats_compare.js index 9e8d7a9aa..e7b1e8985 100644 --- a/scripts/stats_compare.js +++ b/scripts/stats_compare.js @@ -65,13 +65,15 @@ console.log(summaryChange); const layerCountChangeRows = []; for (const layer in difference.layerGroup) { + const stats1LayerCount = stats1.layerGroup[layer]?.layerCount || 0; + const stats2LayerCount = stats2.layerGroup[layer]?.layerCount || 0; const differenceLayerCount = difference.layerGroup[layer].layerCount; const percentageChange = differenceLayerCount / stats1LayerCount; const row = [ layer, - stats1.layerGroup[layer]?.layerCount || 0, - stats2.layerGroup[layer]?.layerCount || 0, + stats1LayerCount.toLocaleString("en"), + stats2LayerCount.toLocaleString("en"), differenceLayerCount.toLocaleString("en"), percentageChange.toLocaleString("en", pctFormat), ]; From 93b2b55dd929c255ef38ca5de28893516049f16c Mon Sep 17 00:00:00 2001 From: Brian Sperlongano Date: Wed, 19 Jul 2023 22:11:20 -0400 Subject: [PATCH 11/15] Remove stray code --- scripts/stats_compare.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/scripts/stats_compare.js b/scripts/stats_compare.js index e7b1e8985..df29d671f 100644 --- a/scripts/stats_compare.js +++ b/scripts/stats_compare.js @@ -81,8 +81,6 @@ for (const layer in difference.layerGroup) { layerCountChangeRows.push(row); } -layerCountChange += layerCountChangeRows.join("\n"); - const layerCountChangeTable = [ ...diffHeaderRow, ...layerCountChangeRows.map((row) => row.join(" | ")), From a1b9deef23cb73928cf6a952c410fb6b85270d35 Mon Sep 17 00:00:00 2001 From: Brian Sperlongano Date: Thu, 20 Jul 2023 22:17:22 -0400 Subject: [PATCH 12/15] Refactor out table row creation and test --- scripts/object_compare.ts | 73 ++++++++++++++++++++++++ scripts/stats_compare.js | 37 +++++------- test/stats_compare/stats_compare.spec.ts | 54 ++++++++++++------ 3 files changed, 125 insertions(+), 39 deletions(-) diff --git a/scripts/object_compare.ts b/scripts/object_compare.ts index 16587681a..d93461cb4 100644 --- a/scripts/object_compare.ts +++ b/scripts/object_compare.ts @@ -70,3 +70,76 @@ function negate(object: object) { return result; } + +// "| | main | this PR | change | % change |", +export type ComparedStats = { + name: string; + beforeValue: number | null; + afterValue: number | null; + change: number; + pctChange: number | null; +}; + +export function statsComparisonRow( + name: string, + val1: number | null, + val2: number | null, + change: number +): ComparedStats { + let pctChange: number | null; + + if (val1 !== null) { + if (val2 !== null) { + pctChange = change / val1; + } else { + pctChange = -1; + } + } else { + pctChange = null; + } + + return { + name, + beforeValue: val1, + afterValue: val2, + change, + pctChange, + }; +} + +const pctFormat: Intl.NumberFormatOptions = { + style: "percent", + minimumFractionDigits: 1, + maximumFractionDigits: 1, + signDisplay: "exceptZero", +}; + +function naLocString(val: number | null) { + return val !== null ? val.toLocaleString("en") : "N/A"; +} + +/** + * produce a markdown row of statistics comparison + */ +export function mdStringValues(stats: ComparedStats): string[] { + const beforeValueStr = naLocString(stats.beforeValue); + const afterValueStr = naLocString(stats.afterValue); + const changeStr = naLocString(stats.change); + const pctChangeStr = + stats.pctChange !== null + ? stats.pctChange.toLocaleString("en", pctFormat) + : "N/A"; + + return [stats.name, beforeValueStr, afterValueStr, changeStr, pctChangeStr]; +} + +export function mdCompareRow( + name: string, + val1: number | null, + val2: number | null, + change: number +): string { + return mdStringValues(statsComparisonRow(name, val1, val2, change)).join( + " | " + ); +} diff --git a/scripts/stats_compare.js b/scripts/stats_compare.js index df29d671f..fd288794c 100644 --- a/scripts/stats_compare.js +++ b/scripts/stats_compare.js @@ -1,4 +1,9 @@ -import { calculateDifference } from "./object_compare"; +import { + calculateDifference, + mdCompareRow, + mdStringValues, + statsComparisonRow, +} from "./object_compare"; const stats1 = JSON.parse(process.argv[2]); const stats2 = JSON.parse(process.argv[3]); @@ -21,33 +26,21 @@ const pctFormat = { signDisplay: "exceptZero", }; -const layersRow = [ +const layersRow = mdCompareRow( "Layers", stats1.layerCount, stats2.layerCount, - difference.layerCount, - ((100 * difference.layerCount) / stats1.layerCount).toLocaleString( - "en", - pctFormat - ), -]; + difference.layerCount +); -const sizeRow = [ +const sizeRow = mdCompareRow( "Size (b)", - stats1.styleSize.toLocaleString("en"), - stats2.styleSize.toLocaleString("en"), - difference.styleSize.toLocaleString("en"), - ((100 * difference.styleSize) / stats1.styleSize).toLocaleString( - "en", - pctFormat - ), -]; + stats1.styleSize, + stats2.styleSize, + difference.styleSize +); -const summaryChangeTable = [ - ...diffHeaderRow, - layersRow.join(" | "), - sizeRow.join(" | "), -].join("\n"); +const summaryChangeTable = [...diffHeaderRow, layersRow, sizeRow].join("\n"); const summaryChange = ` diff --git a/test/stats_compare/stats_compare.spec.ts b/test/stats_compare/stats_compare.spec.ts index 932801176..4ef400e66 100644 --- a/test/stats_compare/stats_compare.spec.ts +++ b/test/stats_compare/stats_compare.spec.ts @@ -1,4 +1,7 @@ -import { calculateDifference } from "../../scripts/object_compare"; +import { + calculateDifference, + mdCompareRow, +} from "../../scripts/object_compare"; import { expect } from "chai"; const a = 3; @@ -19,23 +22,40 @@ const negComplexA = { foo: { a: -a } }; const diffSimpleAB = { a: -a, b }; const diffComplexAB = { foo: { a: -a }, bar: { b } }; +const aStr = a.toLocaleString("en"); + +const simpleAAmdRow = `a | ${aStr} | ${aStr} | 0 | 0.0%`; +const simpleAnullmdRow = `a | ${aStr} | N/A | ${-aStr} | -100.0%`; + describe("stats_compare", function () { - it("tests stats equality", function () { - expect(calculateDifference(simpleA, simpleA)).to.deep.equal(simpleA0); - expect(calculateDifference(complexA, complexA)).to.deep.equal(complexA0); - }); - it("tests stats remove", function () { - expect(calculateDifference(simpleA, null)).to.deep.equal(negSimpleA); - expect(calculateDifference(complexA, null)).to.deep.equal(negComplexA); - }); - it("tests stats add", function () { - expect(calculateDifference(null, simpleA)).to.deep.equal(simpleA); - expect(calculateDifference(null, complexA)).to.deep.equal(complexA); + describe("#calculateDifference", function () { + it("tests stats equality", function () { + expect(calculateDifference(simpleA, simpleA)).to.deep.equal(simpleA0); + expect(calculateDifference(complexA, complexA)).to.deep.equal(complexA0); + }); + it("tests stats remove", function () { + expect(calculateDifference(simpleA, null)).to.deep.equal(negSimpleA); + expect(calculateDifference(complexA, null)).to.deep.equal(negComplexA); + }); + it("tests stats add", function () { + expect(calculateDifference(null, simpleA)).to.deep.equal(simpleA); + expect(calculateDifference(null, complexA)).to.deep.equal(complexA); + }); + it("tests stats diff", function () { + expect(calculateDifference(simpleA, simpleB)).to.deep.equal(diffSimpleAB); + expect(calculateDifference(complexA, complexB)).to.deep.equal( + diffComplexAB + ); + }); }); - it("tests stats diff", function () { - expect(calculateDifference(simpleA, simpleB)).to.deep.equal(diffSimpleAB); - expect(calculateDifference(complexA, complexB)).to.deep.equal( - diffComplexAB - ); + describe("#mdCompareRow", function () { + it("tests md compare same", function () { + expect(mdCompareRow("a", simpleA.a, simpleA.a, simpleA0.a)).to.deep.equal( + simpleAAmdRow + ); + expect(mdCompareRow("a", simpleA.a, null, negSimpleA.a)).to.deep.equal( + simpleAnullmdRow + ); + }); }); }); From e69886b5c175c21f4e24900f351b84443f9d9d57 Mon Sep 17 00:00:00 2001 From: Brian Sperlongano Date: Thu, 20 Jul 2023 22:28:39 -0400 Subject: [PATCH 13/15] Refactor --- scripts/stats_compare.js | 67 ++++++++++++++-------------------------- 1 file changed, 23 insertions(+), 44 deletions(-) diff --git a/scripts/stats_compare.js b/scripts/stats_compare.js index fd288794c..df4392089 100644 --- a/scripts/stats_compare.js +++ b/scripts/stats_compare.js @@ -1,9 +1,4 @@ -import { - calculateDifference, - mdCompareRow, - mdStringValues, - statsComparisonRow, -} from "./object_compare"; +import { calculateDifference, mdCompareRow } from "./object_compare"; const stats1 = JSON.parse(process.argv[2]); const stats2 = JSON.parse(process.argv[3]); @@ -58,26 +53,19 @@ console.log(summaryChange); const layerCountChangeRows = []; for (const layer in difference.layerGroup) { - const stats1LayerCount = stats1.layerGroup[layer]?.layerCount || 0; - const stats2LayerCount = stats2.layerGroup[layer]?.layerCount || 0; - const differenceLayerCount = difference.layerGroup[layer].layerCount; - const percentageChange = differenceLayerCount / stats1LayerCount; - - const row = [ - layer, - stats1LayerCount.toLocaleString("en"), - stats2LayerCount.toLocaleString("en"), - differenceLayerCount.toLocaleString("en"), - percentageChange.toLocaleString("en", pctFormat), - ]; - - layerCountChangeRows.push(row); + layerCountChangeRows.push( + mdCompareRow( + layer, + stats1.layerGroup[layer]?.layerCount, + stats2.layerGroup[layer]?.layerCount, + difference.layerGroup[layer]?.layerCount + ) + ); } -const layerCountChangeTable = [ - ...diffHeaderRow, - ...layerCountChangeRows.map((row) => row.join(" | ")), -].join("\n"); +const layerCountChangeTable = [...diffHeaderRow, ...layerCountChangeRows].join( + "\n" +); const layerCountChange = ` @@ -95,28 +83,19 @@ console.log(layerCountChange); const layerSizeChangeRows = []; for (const layer in difference.layerGroup) { - const stats1LayerSize = stats1.layerGroup[layer]?.size || 0; - const stats2LayerSize = stats2.layerGroup[layer]?.size || 0; - const differenceLayerSize = difference.layerGroup[layer].size; - - const percentageChange = - stats1LayerSize !== 0 ? differenceLayerSize / stats1LayerSize : 0; - - const row = [ - layer, - stats1LayerSize.toLocaleString("en"), - stats2LayerSize.toLocaleString("en"), - differenceLayerSize.toLocaleString("en"), - percentageChange.toLocaleString("en", pctFormat), - ]; - - layerSizeChangeRows.push(row); + layerSizeChangeRows.push( + mdCompareRow( + layer, + stats1.layerGroup[layer]?.size, + stats2.layerGroup[layer]?.size, + difference.layerGroup[layer]?.size + ) + ); } -const layerSizeChangeTable = [ - ...diffHeaderRow, - ...layerSizeChangeRows.map((row) => row.join(" | ")), -].join("\n"); +const layerSizeChangeTable = [...diffHeaderRow, ...layerSizeChangeRows].join( + "\n" +); const layerSizeChange = ` From d04da3fc3b20a45d453539b79108ed5312a05a6b Mon Sep 17 00:00:00 2001 From: Brian Sperlongano Date: Fri, 21 Jul 2023 07:53:50 -0400 Subject: [PATCH 14/15] Consolidate table output --- scripts/stats_compare.js | 39 ++++++++++----------------------------- 1 file changed, 10 insertions(+), 29 deletions(-) diff --git a/scripts/stats_compare.js b/scripts/stats_compare.js index df4392089..99c5a0155 100644 --- a/scripts/stats_compare.js +++ b/scripts/stats_compare.js @@ -35,16 +35,7 @@ const sizeRow = mdCompareRow( difference.styleSize ); -const summaryChangeTable = [...diffHeaderRow, layersRow, sizeRow].join("\n"); - -const summaryChange = ` - -## Style size statistics - -${summaryChangeTable} -`; - -console.log(summaryChange); +printTable("Style size statistics", [layersRow, sizeRow]); /** * Show comparison of the number of layers in each group before and after @@ -63,18 +54,7 @@ for (const layer in difference.layerGroup) { ); } -const layerCountChangeTable = [...diffHeaderRow, ...layerCountChangeRows].join( - "\n" -); - -const layerCountChange = ` - -## Layer count comparison - -${layerCountChangeTable} -`; - -console.log(layerCountChange); +printTable("Layer count comparison", layerCountChangeRows); /** * Show comparison of the aggregate size of layers in each group before and after @@ -93,15 +73,16 @@ for (const layer in difference.layerGroup) { ); } -const layerSizeChangeTable = [...diffHeaderRow, ...layerSizeChangeRows].join( - "\n" -); +printTable("Layer size comparison", layerSizeChangeRows); -const layerSizeChange = ` +function printTable(headingText, rows) { + const table = [...diffHeaderRow, ...rows].join("\n"); + const text = ` -## Layer size comparison +## ${headingText} -${layerSizeChangeTable} +${table} `; -console.log(layerSizeChange); + console.log(text); +} From 9eee05032464dcba6aefc725d3ebaae814394b96 Mon Sep 17 00:00:00 2001 From: Brian Sperlongano Date: Fri, 21 Jul 2023 07:57:00 -0400 Subject: [PATCH 15/15] Remove unused code --- scripts/stats_compare.js | 7 ------- 1 file changed, 7 deletions(-) diff --git a/scripts/stats_compare.js b/scripts/stats_compare.js index 99c5a0155..45f70dad7 100644 --- a/scripts/stats_compare.js +++ b/scripts/stats_compare.js @@ -14,13 +14,6 @@ const diffHeaderRow = [ * Show comparison of overall aggregate statistics between this PR and previous */ -const pctFormat = { - style: "percent", - minimumFractionDigits: 1, - maximumFractionDigits: 1, - signDisplay: "exceptZero", -}; - const layersRow = mdCompareRow( "Layers", stats1.layerCount,