Repository navigation
Rust IP Intelligence generator writes Float and WktString as the crates declare them - #47
Draft
jwrosewell wants to merge 3 commits into
Draft
jwrosewell wants to merge 3 commits into
jwrosewell wants to merge 3 commits into
Conversation
The Rust IP Intelligence builder published a Single property in the property type table as PropertyValueType::Double, because the Rust core had no single precision type when the builder was written. The Rust core now has PropertyValueType::Float, and 51degrees-rust declares Latitude and Longitude with it by hand in ip-intelligence-shared/src/ip_intelligence_data.rs (51Degrees/51degrees-rust@98a88ba), as the IP Intelligence data model and .NET type them float. A generated file now carries the same declaration, so the next generation does not undo that edit. The accessor (f32) and the store getter (float) were already right. The comment that explained the old mapping is removed, as it is no longer true.
The Rust IP Intelligence builder gave a WKBR property, which is Areas, as a plain String read by the string getter and published as PropertyValueType::String. 51Degrees/51degrees-rust#345 (a draft) gives Areas the WktString type the IP Intelligence data model names, as .NET does, and changes the generated ip_intelligence_data.rs by hand to match. This makes the builder write the same: the accessor returns AspectPropertyValue<WktString>, the impl reads it with wkt_string, the property type table publishes PropertyValueType::WktString, and the file imports WktString from fiftyone_pipeline_core. The C# builder already maps WKBR to WktString. A weighted WKBR property keeps the weighted string list, because the Rust data base reads every weighted property as a list of strings and no property in the metadata is a weighted WKBR. This needs WktString in fiftyone_pipeline_core, which comes with 51Degrees/51degrees-rust#345, so it merges only after that pull request does and only if it keeps the WktString type.
This was referenced Nov 8, 2026
The review of 51Degrees/51degrees-rust#345 kept IpIntelligenceData::areas as AspectPropertyValue<String>, because releases of the crates move only the patch number and Cargo's SemVer guide treats a changed trait item signature as a major change. Areas is still held as a WktString, so the property type table keeps PropertyValueType::WktString for WKBR, and the accessor goes back to String read by the string getter, which gives the WKT value's text. WktString leaves the file's imports, as no accessor returns it. The C# builder is unchanged and still gives WKBR as WktString.
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.
The PropertyGenerator's Rust IP Intelligence builder writes
ip-intelligence-shared/src/ip_intelligence_data.rsin 51degrees-rust. That file has been changed by hand twice since it was last generated, and the builder does not yet write either change, so the next generation would undo both. This pull request makes the builder write them, in three commits. The first can go on its own, and the second and third go together.Singlepublished asFloat(dbfd02c). The property type table gave aSinglepropertyPropertyValueType::Double, because the Rust core had no single precision type when the builder was written. The core now hasPropertyValueType::Float, and 51Degrees/51degrees-rust@98a88ba declares Latitude and Longitude with it, as the IP Intelligence data model and .NET type themfloat. 98a88ba is on 51degrees-rust'srelease/dotnet-paritybranch, which goes into the next Rust release. The accessor (f32) and the store getter (float) were already right, and the comment explaining the old mapping goes because it is no longer true.WKBRgiven asWktString(f2ca14f), changed by commit 3. Areas, the onlyWKBRproperty, was a plainString. Draft 51Degrees/51degrees-rust#345 gives it theWktStringtype the IP Intelligence data model names, as .NET does, and edits the generated file to match. The builder now writesAspectPropertyValue<WktString>, reads it withwkt_string, publishesPropertyValueType::WktStringand importsWktStringfromfiftyone_pipeline_core. The C# builder already mapsWKBRtoWktString. A weightedWKBRkeeps the weighted string list, because the Rust data base reads every weighted property as strings, and no property in the metadata is a weightedWKBR.WKBR's accessor kept asString(c4d05b4). The review of 51degrees-rust#345 keptIpIntelligenceData::areasasAspectPropertyValue<String>. Releases of the crates move only the patch number, and Cargo's SemVer guide treats a changed trait item signature as a major change, which a caller's default requirement such as"4.5.2"would take on the nextcargo update.Areasis still held as aWktString, so the property type table keepsPropertyValueType::WktStringforWKBR. The accessor goes back toStringread by thestringgetter, which gives the WKT value's text, andWktStringleaves the file's imports because no accessor returns it. Together, commits 2 and 3 change only the table type, and the C# builder still givesWKBRasWktString.Merge order
PropertyValueType::WktString, which only 51degrees-rust#345 adds tofiftyone_pipeline_core, so they merge together and only after 51degrees-rust#345 does. The question 51degrees-rust#345 asked, whetherareas()may change fromStringtoWktStringin a patch release, is settled as no, which commit 3 follows.How it was checked
Nothing was built or run locally, because local builds on this machine are paused while disk space is short. This repository's Pull Request workflow builds the PropertyGenerator.
The generator could not be run, so its mapping was checked against the metadata instead.
IpandIpV6echo properties) were listed with their value types from the embedded metadata in the cached FiftyOne.MetaData 5.1.77 package, by loading the DLL in PowerShell withMetaDataServiceoverEmbeddedResourceService. Latitude and Longitude are the onlySingleproperties and Areas the onlyWKBRone.release/dotnet-parity(217a8ed)rcbefore this changeSo the old rules give the file as first generated, commit 1 gives the
Floatedit, commits 1 to 3 give 51degrees-rust#345's file as it now stands, and no other property changes. 798c8a0 is 6f0e937 withrelease/dotnet-paritymerged in and the review's changes, and itsAreasisAspectPropertyValue<String>read bystringwith the table typeWktString.Not in this change
RawWeightingandValuerather thanvalueandweight). If that changes the core type the table publishes for a weighted property, the builder'sKeyValueListand its comment change with it, in a pull request of its own.Singletof64andDoublethroughout, and the Device Detection file has had no hand edit, so it is unchanged.CI
On c4d05b4, Pull Request run 42492501817 passed. "Build PropertyGenerator" built with 0 errors and the same 4 warnings at the same lines as below, and "Build CopyrightUpdater" with no warnings. Only rc into main passed.
Earlier, on f2ca14f, Pull Request run 42482237555 passed. "Build PropertyGenerator" built with 0 errors and 4 warnings, being CS8714 at
ClassBuilderBase.csline 74, CS8618 and CS0169 onIpIntelligence._properties, and CS8603 atGeneratorBase.csline 63. These are the same four warnings at the same lines asrc's own run on 3e68c92 (41697793840), so this change adds none. "Build CopyrightUpdater" built with no warnings, and Only rc into main passed. The workflow builds the generator but does not run it, so the table above is the check of what it writes.