Repository navigation
Offer the derived properties with a derived property element - #98
Draft
jwrosewell wants to merge 8 commits into
Draft
jwrosewell wants to merge 8 commits into
jwrosewell wants to merge 8 commits into
Conversation
…rty element The tests run every case file and every rejection case in the submodule's tests folder through a pipeline, and test the element's settings, logging, metadata and the checks it makes when a pipeline is built. The element itself follows in the next commit.
fiftyone.pipeline.derivedproperty runs the scripts of derived-properties, built into the package from the submodule's scripts folder or given as files or text, and writes one property for each under derived. lib holds the reference parser, validator, evaluator and canonical printer of derived-properties as CommonJS. The package is tested but not yet published.
This was referenced Nov 14, 2026
…stems The default file systems of both ignore letter case, as the Rust crate's wildcard does on both, and Node.js ignored it on Windows only.
…dows macOS's default file system ignores letter case, as Windows's does, so a wildcard now matches names in any case there too, as the Rust crate's does.
This was referenced Nov 14, 2026
Draft
Draft
… both lint exclusions
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.
Node.js had no element for the derived properties that derived-properties defines, so a Node.js pipeline could not offer HumanConfidence or IsGdpr, whilst .NET, Rust and PHP can. This adds a
pipeline/fiftyone.pipeline.derivedpropertypackage whoseDerivedPropertyElementruns every script in thescriptsfolder of derived-properties, and tests that run every case file in itstestsfolder, the scripts undertests/invalidincluded.Closes #80.
What the specification says
All quotes are from derived-properties at cc306386, the head of its
mainand the commit the new submodule pins.docs/format-1.md, lines 4 to 6:
The one rule, lines 35 to 39:
When a property is absent, lines 451 to 455:
Lines 491 to 497, on an element added after a source it reads:
Writing YAML, lines 562 to 570:
Two places where the languages are allowed to differ, lines 590 to 605, being the line a fault names, and how strictly a JSON script is read:
docs/testing.md, lines 3 to 6:
Lines 177 and 196 to 197:
docs/authoring.md, lines 365 to 384 asks for one information line per script at build naming "the script, its version, the format number, where the script came from (a built in name, a file path or code) and the output property", and at debug level "the compiled model as canonical JSON". Lines 441 to 442 add a warning for a deprecated script. Lines 452 to 464 describe the check when the element is added, where "a source property that no earlier element supplies" and "two elements in one pipeline producing the same derived property name" fail the pipeline build. The word "earlier" there is at odds with format-1.md lines 491 to 494 above, and .NET, Rust and PHP accept an element that supplies the property wherever it sits, so Node.js follows the format reference, which calls itself the contract.
README.md, lines 80 to 109 names the build parameters
ScriptsandScriptFiles, each "one string with commas between the names", where "a name that holds a comma is wrapped in double quotes".What changed
The reference implementations read were .NET,
pipeline/DerivedPropertyof 51degrees-dotnet, Rust, thederived-propertiescrate of 51degrees-rust, and PHP,pipeline/derivedpropertyof 51degrees-php. The Node.js code is the reference tools the specification follows,tools/parse.mjs,validate.mjs,evaluate.mjsandcanonical.mjs, rewritten as CommonJS in this repository's style with the same logic, fault paths and messages. Files are linked at the head, a8fb0e48..gitmodulesadds derived-properties aspipeline/fiftyone.pipeline.derivedproperty/derived-propertieswith the relative URL../derived-properties, pinned at cc306386. The package reads the built in scripts from it directly, so there is no copy to keep in step..eslintignoreleaves the submodule out of the lint.filesin the package'spackage.jsonandremote_package.jsonpacks the package's own code,liband the submodule'sscriptsfolder, and nothing else from the submodule.^4.1.0and reads with itsCORE_SCHEMA, soyesstays the string"yes"as the reference tools read it (lib/parse.js). JSON scripts are read withJSON.parse, the strict reading the specification allows.DerivedPropertyElementwrites oneAspectPropertyValueper script underderived, throughDerivedPropertyData, where names are matched without regard to case. A script with a source property missing has no value, and readingvaluethrows the message naming every missing property, which is the Node.js pipeline's no value error. It reads a source as Node.js elements hold it, carrying the source's ownnoValueMessage, or the message of the error an element throws for a property it does not have, and turning a weighted list into theValueandWeightshape the evaluator takes. A script reads what an earlier script of the element wrote. A request with no element data at all under a key a script reads is logged once for each script, as format-1.md lines 491 to 497 ask.scriptsandscriptFilessettings, which a pipeline configuration file gives, matched without regard to case, soScriptsandScriptFileswork as the README names them.DerivedPropertyElementBuilderoffersaddBuiltInScript,addScriptFileandaddScriptin code, as .NET's builder does. A file name with*or?adds every file in its folder whose name matches, in sorted order, ignoring letter case on Windows and Apple systems, whose default file systems ignore it, as the Rust crate does. Every fault in every script is reported in oneScriptValidationError. A setting the element does not have is refused, so a setting written wrongly cannot leave its scripts out without a word, and an element with no script is refused with a message saying how to give one.onRegistrationrefuses the pipeline with the same messages as .NET, Rust and PHP where nothing supplies a source property or another element writes the same derived property, with the elements that run in parallel opened out. Where an element sits is not judged, and an element that lists no properties may supply any, because a cloud engine learns its properties from its first response. The line and the canonical JSON authoring.md asks for, and a warning for a deprecated script, are logged on the next tick, because the pipeline is still being built whenonRegistrationruns and nothing can be listening to its log yet.name,type,category,descriptionandisList, followed by the rest of the script'sOutputblock.package.jsonadds the package to the workspaces, sonpm installlinks it, the build's lint covers it and the unit tests run its tests. It is not inci/packages.json, which lists the packages Publish releases, so Publish leaves it out. AGENTS.md, the README andpipeline/readme.mddescribe the package and the submodule.The tests, in
pipeline/fiftyone.pipeline.derivedproperty/tests, 224 in all:sharedCases.test.jsruns the 69 value cases (57 for HumanConfidence and 12 for IsGdpr) twice, once with the source values asAspectPropertyValueand once as plain values, each through a real pipeline with a source element giving the case's properties and the script the package carries, and the 29 rejection cases, each refused with a fault at every expected path and mentioning every expected fragment. A missing value must name exactly the properties the case lists.builtInScripts.test.jschecks that the built in scripts are the submodule'sscriptsfolder and builds one element with every one of them.pipelineCheck.test.jsfollows the Rust crate'stests/pipeline_check.rs.element.test.jsandscripts.test.jscover reading the values Node.js elements give, logging, settings (a wildcard's letter case on each platform included), metadata, the JSON Builder, YAML and JSON, type inference and conversion.Runs
Each throwaway run uses a workflow that installs the workspace, lints the package with ESLint and runs its tests with Jest, on Ubuntu with Node.js 22 and 24 and on Windows with Node.js 24. A fourth job packs the package with
remote_package.jsonin place, as Publish would, lists the tarball, installs it into an empty project with its dependencies from npm, and runs the built in scripts.Cannot find modulefor the element, the builder,lib/validateor the package itself, and the pack job failed withCannot find module '.../fiftyone.pipeline.derivedproperty/index.js'.rcwas merged in, and nothing else changes. Each test job passed the lint and ran "Tests: 221 passed, 221 total". The pack job packed 14 files, among themderived-properties/scripts/HumanConfidence.yamlandIsGdpr.yamland no other file of the submodule, installed the tarball into an empty project, built both scripts asHumanConfidence 0.8.0 from built in HumanConfidenceandIsGdpr 0.1.0 from built in IsGdpr, and answered IsGdprtrueforFR.no-irregular-whitespace). c155270 tests for it by its code instead, and adds a description to five constructors.rcwas merged in twice, at a8fb0e48 and at the head 667d9db8, the second taking #37's version per package, each with no conflict and no change to the package. Give each package its own version and release only what changed #37 lists the packages Publish releases inci/packages.json, which does not name this one.darwinas well.darwincase failed ("Tests: 1 failed, 223 passed, 224 total"), its wildcard*.YAMLmatching neitherBand.yamlnorOther.yaml.tmp/pr-matrix-98d, success. Configure, all six Build and Test jobs (Ubuntu x64 and ARM and Windows, Node.js 22 and 24) and Complete are green. All 7Setting 'PullRequestSha'lines name the head 6efb0657, and Configure printed "Left out 2 macOS entries".tmp/pr-matrix-98c, success. Configure, all six Build and Test jobs (Ubuntu x64 and ARM and Windows, Node.js 22 and 24) and Complete are green. All 7Setting 'PullRequestSha'lines name 667d9db8, and Configure printed "Left out 2 macOS entries". An earlier full build on a8fb0e4, run 43560055156, was green too once its Windows Node.js 24 job was run again, the first attempt having failed to start Chrome in the Selenium contract tests ("session not created from disconnected: unable to connect to renderer").macOS was left out of this draft's full build under the 9 November decision. The rc head is built with macOS before it merges into main.
What remains
ci/packages.json. When to publish a new package is a separate choice.filesnames the package's own code as*.js. npm reads a pattern with no slash by file name at every depth inside a folder that anotherfilesentry opens, so a.jsfile at the top of the derived-properties submodule would be packed too. derived-properties has no.jsfile today, and the green run's pack lists none. Anchoring the pattern as/*.jsis the fix #78's change makes for the OWID library, and is left for when the package is first published.getValueAsBool, which answersvalue == "True"(ResultsBase.cpp lines 118 to 140), so a Boolean property stored asUnknownreaches Node.js asfalse. HumanConfidence comparesdevice.IsVisibleanddevice.HasWebDriverwith"Unknown", and where the data file types those properties as Boolean that comparison is never true through that engine. PHP found the same and raised it as 51degrees-php#95. It needs a data file to check, so it was not checked here.Output.Namenaming that element. Format 1 as published at cc306386 asksOutput.Nameto match^[A-Za-z][A-Za-z0-9]*$, so Node.js follows the format and has no such scripts.