[235] Rename unclear identifiers across frontend and backend (high-value pass) - #236
Draft
CarsonDavis wants to merge 12 commits into
Draft
[235] Rename unclear identifiers across frontend and backend (high-value pass)#236CarsonDavis wants to merge 12 commits into
CarsonDavis wants to merge 12 commits into
Conversation
8 tasks
…djacent functions)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #235
What this is
A readability pass over
src/essenceandAPI/: ~230 identifier renames across four passes, behavior-preserving throughout, with one deliberate schema exception (thetileFormatconfig key, which ships with a backward-compat alias — see its section below). Focused on the highest-leverage cases per the issue — single-letter or misleading names holding domain objects, the config-vs-instance ambiguity (s/layerObj/lall meaning "layer" in different senses), and names people already had to explain in review — not an exhaustive sweep of every loop index.How the list was built
Pass 1 — the base inventory. Eight area-scoped inventory passes collected 199 candidates; a synthesis pass unified them under one vocabulary (one concept = one name everywhere); an adversarial review then attacked every entry (string coupling to selectors/DOM ids, persisted-schema property keys, shadowing, misread meaning, low value) and rejected or amended the weak ones. 151 renames survived review; 150 were applied (one was skipped during application because two approved renames in
DrawTool_Files.jsturned out to be mutually exclusive — the stronger one was kept). Three further passes followed; each has its own section below.Second review pass (post-PR)
After the PR was opened, the full diff went through a second adversarial review: 8 fresh per-area reviewers (two of them verified via AST lockstep comparison that the diff is structurally identical to base apart from renames) plus a 3-judge name-quality panel (accuracy / consistency / readability). Result: zero must-fix findings and three independent 'these are good renames' verdicts. The actionable findings were consistency completions — correct renames that had stopped at a file boundary, leaving twin code on old names — and are applied in the follow-up commit:
Post-review consistency completions — twin files and adjacent functions brought onto the same vocabulary
layerObjlayerConfiglayerConfigfinalRadfinalRadiuslayerObjNamelayerConfigNamel(getLayerOpacity)mapLayerd/dNext(getSublayers ×2)layerConfig/sublayerConfigsd(getListOfUsedGeoDatasets, dataFlat.forEach)layerConfiglayerData(transformStacUrl, getUrl, getDynamicProps)layerConfiglayerObj(updateVectorLayer)layerConfigrasterparsedGeorastergeorasterimportvectorImageIndicesvectorAndImageIndicespPpanelPercentspanePercentsURL string untouched)s(2nd site)selectedPartsnode(depthTraversal)layerConfigslayer(setLayerWmsParams + 5 config locals)layerConfiglayerData/ for-inlayerlayerConfig/layerNamecmlselectionEntryt(2nd forEach)templateFieldtemplatetemplateFieldsf(addShapeToList)drawnLayer+featureLayersp,c,dl,dlc,val, innerc,layerUrlurlParts,scratchCanvas,tileDataUrls,tileCanvases,resultRow/visibilityCode,pixelColor,tileDataUrlsdv,d,dshadeData,tileIndex,heightGridobserverHeightsourceHeighttileCanvasscratchCanvastileCanvasesat the assignment sitelayerNamelayerUUIDlayerUUIDin drawLegends/overwriteLegends in this fileDeliberately deferred (so the asymmetry is not read as an oversight)
layerObjin Map_.js (~310 uses) — a defensible name there; renaming it is a large mechanical sweep worth its own pass.layer.layerObjproperty key in LayerConstructors.js pathGradient — property keys are out of scope by rule.layerDatain files the review did not flag (Map_.js:361, MetadataCapturer.js, LayersTool.js, other Description.js functions).refreshFile(id, …)in DrawTool_Files.js — renaming the outeridtofileIdwould be captured by the innerrenderFileFeatures(fileId, …)parameter inside two closures (verified: the closure reads the outer binding, and the values differ — raw argument vs parsed id). Left as-is on purpose.Third pass — names people had to explain
A final sweep targeted the highest-signal rename class: names someone already paid a confusion cost for on record. Sources: (1) the last ~40 PR review threads mined for name-confessions, (2) the codebase's own comments mined for apologies (
//sf for search field), (3) mechanism-names — identifiers named for how a value is computed instead of what it means. Every candidate went through the same adversarial vetting gate (25 survived: 19 as proposed, 6 amended, 0 rejected).The two that came straight from PR #213 review:
performTimeUrlReplacements→applyUrlReplacementsAndCacheBust— the pipeline doc literally said "Despite the name, it does not substitute the {time}/{starttime}/{endtime} family" and the review reply was "don't call it that then, lmao". Renamed; the doc's apology paragraph is rewritten to a plain description.splitColonType→tileSourceType— "can we rename splitcolontype to something that actually indicated the value?" It holds which tile-source scheme a layer URL uses (stac-collection/COG/titiler-url). This is an internal-only object key, verified contained to the tile pipeline modules and tests.Confession-driven renames — 25 vetted entries, grouped into the rows below
nextUrlcompiledUrlsplitColonLayerUrl,splitColonUrl+splitParams,urlSplitRaw+urlSplit×2,urlSplit×3,splitsourceUrlParts,stacUrlParts+collectionNameAndParams,urlParts+urlPartsLower×2,urlParts/hrefParts×3,urlPartssf,strsearchFieldPairs,searchFieldParamString//sf for search field) — comment deletedisFloat(param)fromFloatingPanelsArraycPx,tPx,cCost,cLatLng,tLatLng,tPxIndex,dicurrentPixel,neighborPixel,currentCost,currentLatLng,neighborLatLng,neighborHeapIndex,imgDataIndextPxwas verified to be the Dijkstra neighbor, not the 'target' the old comment claimedtUrl,oSplittoolStateParts,orderTripletUrlheld tool-state parts, not a URL — the name liedrawTimeutcTimerawDOM attribute it feeds stays)parsedIdtargetFileIdp(10 sites)metaPropsproperties._meta bag, the module's most-manipulated objectsplit/start/end;bestVal,featuresVal,numVal,extractedVal[prefix, suffix]destructure;nextAvailableNumber,existingFieldValue,requestedNumber,otherFeatureNumberfeaturesValholds the raw string, not a numberretlayerActionfound,resultmatchIndex,matchedUuidEntryfoundsounded boolean but held a findIndex resulturlSplit,relUrlSplit,split,t,ntimeDirPath,dirCacheKey,dirNameParts,timestamp,label{t, n}response keys are external shape and stayfilterSplit,fSplit,spatialFilterSplit,vSplit×2filterEntries,filterParts,spatialFilterParts,pathSegmentsinitialendSplit,d,s,opMult,dateAddSec,dateStageddateAndOffsetParts,timeString,hmsParts,offsetSign,timeWithOffset,parsedDatearr,slayerEntries,nameAndOpacitystrippedpathWithoutExtsplitPathpathSegmentsThe
tileFormatschema rename (deliberate exception)tileformat→tileFormat— the one externally-visible rename in this PR, explicitly approved by the stakeholder ("missions can be updated"; also flagged in #213 review: "why is tileformat not camelCase?"). What shipped:auxiliary/bulk_tiles.pyconfig generator.parseConfig/expandLayers) — old missions keep rendering with zero manual edits and convert on their next save. Both alias lines are commented as removable once stored configs are migrated.demtileurl,demparser,scalefactor,strokecolor,nointeraction, legacyvisibilitycutoff, ~46 mission-level tab keys) are enumerated in a drafted follow-up issue using the same alias pattern.What is deliberately NOT touched
tileFormatconfig key, detailed below); existing missions and permalinks are unaffected.L_,F_,Map_, …) — explicitly out of scope per the issue.tileFormatschema change.dx/dy, haversinea/c,sx/cx) — where non-obvious, a one-line comment names the formula instead.Verification
npm run typecheck— cleannpm run test:unit— 889/889 tests pass (57 files), matching the pre-change baselinenpm run build— production build succeedsEvery rename in the per-area tables at the end also names what the value actually holds, so the list is reviewable without reading the diff. Where a later pass amended a pass-1 name, the table shows the final name with the intermediate in parentheses.
Naming conventions (issue #235)
The rules below governed the variable/parameter/private-function renames. Aside from the two documented key exceptions (
tileSourceType, an internal-only key, and thetileFormatschema change), no property key, DB column, request/response field, URL parameter, DOM id, CSS class, orwindow.*global is touched.Vocabulary
L_.layers.data,configData.layers)layerConfig/layerConfigss,d,l,node,layerObj,layerDatalayerNamel,n,s,layer_layerNamePartssL_.layers.layer[name]mapLayerlfeatureLayer,featureLayers,featureLayerIdl,g,e,l2,_layerfileLayers,drawnLayerl,lg,f,layersublayerName,sublayersub,sfeature,geojson,coordsf,d,g,c,data<qualifier>LatLng,lngLatPointwhen x = lngll,llM,xy,ell<role>El,<role>Input,<role>Canvast,c,n,bb<tool>Data,<tool>Stated,dvtemplateField,templateFieldst,templatefileIdid,indexselectionEntry,selectionIndexl,cml,c{layer, index, fileid}locatorfeatureLocatorlifprop:<name><prop>OrPropcol,fiC,fiO,opa,wei,radon<Event>,<what>Callbackadd,f,d<what>Responsed,s,ressql,timeClauseq,t<param>Parts,<thing>Tokens,searchTermsp,s,string,fieldString,xRules
layerConfigis the persisted JSON,mapLayer/featureLayeris the thing built from it,layerNameis the string key between them. Neverlfor all three.layerUrlholding a data-URL map,selfishmeaning "this is the retry",indexmeaning a file id,resmeaning a response payload in Express code, singular names holding arrays.sql,coords,bbox,ctxwhere it really is a canvas context). They are not fine when the declaration is more than a screen from its use — that is the actual test, not letter count.i,j,k,x,yin numeric loops;e/errin catch blocks. But a for-in key that is a layer name, a UUID, or a file id is not an index and gets named for what it holds.dx/dy/dz,sx/cx/sy/cy, plane coefficientsa/b/c/d, haversinea/c, rasteri/jscans. Where the formula is non-obvious the fix is one comment line naming it, not a rename.sed. Several targets are single letters that also appear inside string literals in the same file:'v'is a live DOM attribute inDrawTool_Editing.js,'rad'is an angle unit inLayerConstructors.js,'s'is a pluralization suffix inLayerGeologic.js,var(--color-l)inLegendTool.js,xyorll: 'xy'inMeasureTool.js,.val(inViewshedTool.js, and@param {string}JSDoc inDrawTool_Files.js. Repo files are CRLF, so edits must be byte-safe or the diff explodes to whole files.Deliberately out of scope
Module aliases (
L_,F_,Map_,Globe_,L,$, …);layerObjinMap_.js(~310 uses — a defensible name there; theLayerConstructors.jstwin WAS unified tolayerConfigin the second pass, leaving only thelayer.layerObjproperty key untouched); and, beyond the two documented exceptions, all property keys, persisted schema, API payload fields, permalink field order, DOM ids and CSS classes.Adjacent defects found while reading — NOT fixed here; an issue is drafted for the first, the rest are queued for filing:
Layers_.js:3082references an undeclaredlayerObj(latent ReferenceError on an error path);LayersTool.js:1487recurses withnode[0]wherenode[i]is meant; several dead locals (ctxin fivemake*Layerfunctions, unusedbbbounds blocks,tsin both*Tool_Manager.js,makeDataLayer's unused third parameter,Layers_.modifyLayer's unuseddata).Renames by area
layers-core — src/essence/Basics/Layers_/ — 25 renames
slayerConfigdlayerConfigsslayerConfigsubsublayerNamedlayerConfiglfeatureLayergfeatureLayers_layersmap, whose values are per-feature layers with .feature and ._latlnglfeatureLayerId_layersmap (a string id) — the layer is g[l]. Directly misleading, sincelmeans a construlfeatureLayerppropNamename:of the returned image descriplmapLayerlayerObjlayerConfigslayerConfigradradiusOrPropsfeatureStylellayerConfigldlayerConfigsfiCfillColorOrPropcolcolorOrPropsgeologicStyleggeologicStylesat L89 (g.type, g.tag, g.color, g.size, g.rot) — one concept, two onopaopacityOrPropweiweightOrPropfiOfillOpacityOrPropslayerNamePartsmap-core — src/essence/Basics/Map_/Map_.js — 16 renames
ctxmapContextdatageojsonhasIndexvectorAndImageIndices(viavectorImageIndices)bbconfigBoundsbounds:option. The samegeorasterparsedGeoraster(viaraster)fieldStringfieldTokensminbandMinaddonGeoJSONFetchedpclickPointssublayerNamefonExtentChangehasIndexRasterrasterIndicesllayerNamekeyAsNamefeatureNameValueelllatlngEvent_layerfeatureLayersdraw-main — DrawTool_Editing.js + DrawTool_Drawing.js — 16 renames
ddrawStatevcolor / opacity / dashArray / weight / fillColor / fillOpacity / symbol / radius / width / length / lineCap / lineJoin / fontSize / rotation (one per setter, in declaration order L1339/1457/1542/1610/1724/1830/1898/1928/1993/2059/2125/2191/2257/2298)layertargetLayersshapeLayercselectionIndexddrawStatelselectionEntrylgfileLayersnnewNameInputhswatchHtmllselectionEntryslselectionLayerlfeatureLayeredrawnLayerereads as an event everywhere else incheaderNameElctxdrawEventdraw-rest — DrawTool.js, _Files, _Shapes, _Templater, _Publish — 21 renames
ttemplateFieldttemplateFieldlfileLayersldrawnLayerlayerlayerNamel2featureLayer_layersKEY string, then is immediately reassigned tollayerNamevar s = l.split('_')on the next line and passed to addFfdrawnLayeridresolvedFileIdfileIdparameter (programmatic call) or the file_id attribute of theefeatureLayersissuedfileResponsellayerNameindexfileIdtargetFileId, formerlyparsedId); used as 'DrawTool_' + index, DrawTool.fileGeottemplateFieldkeepGoingrenderFileFeaturesefeatureLayerlayerfileLayersfor (var i = 0; i < layer.length; i++)reading layer[i].fslayerNamePartslayerfileLayersDrawTool_${fileId}]; forwarded to _validateIncremtemplatetemplateFieldsviewshed-shade — 20 renames
oobserverCellotargetSourceCelldshadeDatadviewshedDataggridsggridspurlPartsdataHcellTargetHeighta / b / c / d / a1 / b1 / c1 / a2 / b2 / c2(comment)dviewshedDatasaerResponsedheightGridcpixelColordtileIndexcscratchCanvas(viatileCanvas)dltileDataUrlsdlctileCanvasesdvviewshedDatad) — .topLeftTile, .source, .zvalvisibilityCodelayerUrltileDataUrlstools-misc — src/essence/Tools except Draw/Viewshed/Shade — 18 renames
nodelayerConfigstlistItemElllayerUUID(vialayerName)vrawValueglineCoordsnlayerNamessublayerNamerAmroundAmountnodelayerConfigsflayerFiltersd2refreshedDataSourcedatalayerConfignodelayerConfigslllngLatPointtemppointMarkerllMmarkerLatLngselfishisRequerylfeatureLayerbasics-misc — src/essence/Basics (excl. Layers_ and Map_) + src/essence/Ancillary — 21 renames
pppanelPercentsffeaturesx / cx / sy / cy / sz / cz(comment)lfeatureLayers_layersdict; every use is l[i].feature.geometry.cooimageConfigsselectedPartsselectedURL parameter: [layerName, lat-or-key, lon-or-value, view, zoom]gcoordssfsearchFieldSpecsllayerConfigxsearchTermseendTimestampsstartTimestamplclickedFeatureLayerdoXactionvlayerVariablesccoordslmapLayerrurlReplacementllayerConfiglsparklineLayerffeaturefeatureparameter, used for booleanIntersects(bounds, f), bbox(f), f.propeapi — API/ — 13 renames
qsqlffeatureRowffilterttimeClauseitemstoolItemsttemplateFieldrfeaturesnodelayerConfigswhwebhookwebhookparameter of drawFileUpdate/drawFileDeletestrinputresfileResponselayerfeatureslet geojson = layer[i],iuuid