refactor: new pyRevit C# config service - #3450
Draft
ChrisCrosley wants to merge 66 commits into
Draft
Conversation
Port the config abstraction from pyrevitlabs#2482 (dosymep), scoped to INI only; Json/Yaml backends omitted. Net-new assemblies plus wiring: - Directory.Build.targets: map net8.0 -> netcore. - pyRevitLabs.sln: register both projects. - .gitignore: un-ignore Configurations.Ini/ (matched by *.ini on case-insensitive filesystems).
Ports the ConfigurationService and IniConfiguration test projects from pyrevitlabs#2482. Json/Yaml test projects omitted with their backends.
Rewire the CLI and pyRevitLabs.PyRevit config consumers onto the new configuration service; remove the bespoke CLI config reader. - PyRevitConfigs: rewritten over IConfigurationService / typed sections. - Remove pyRevitLabs.PyRevit/PyRevitConfig.cs (superseded). - Port PyRevitAttachments, PyRevitCaches, PyRevitClones, PyRevitExtensions. - csproj: reference the Configurations assemblies (Json backend dropped), keep develop's LibGit2Sharp 0.31.0. Reconciled against current develop (3-way merge, not a straight port): - Preserve develop's install-scope ConfigFilePath (IsInstallAllUsers marker) over the PR's simplification. - Re-apply develop's attachment session cache (GetAttachedCached / ClearAttachmentCache) and clone bin-artifact install + --skip-bin. - Add close-output config (GetCloseOutputMode/GetCloseOtherOutputs + OutputCloseMode enum + CoreSection keys) consumed by ScriptConsole.
Replace the Python-side config reader with a thin wrapper over the shared C# configuration service. - userconfig.py: PyRevitConfig now wraps IConfigurationService; typed Core/Routes/Telemetry section access; _SectionCompatWrapper preserves get_option/set_option for extensions. Module init no longer runs the upgrade normalize loop or an unconditional save_changes(): opening Revit no longer rewrites the ini. - configparser.py: ConfigSection/ConfigSections over the service; the JSON-over-INI fixup chain moves to the C# IniConfiguration backend. - labs.py: drop MadMilkman.Ini (package removed); reference pyRevitLabs.Configurations / ConfigurationService. - revit/tabs.py: tolerate non-string/malformed config values. - loader/sessioninfo.py: drop config_type/config_file log line. The PR's unrelated runtime DLL-resolution change is intentionally not ported. Settings reconciliation (new_loader, read_script_metadata, output close mode) follows in the next commit.
…bs#2482 base Re-add config surface the PR predated, so existing consumers (sessionmgr, Settings dialog) keep working: - CoreSection: new_loader, read_script_metadata typed keys (close-output keys were added with the C# migration commit). - userconfig: new_loader, read_script_metadata, output_close_others, output_close_mode_enum properties (typed Core access). - userconfig.get_thirdparty_ext_root_dirs: restore pyrevitlabs#3193 deterministic ordering (default path first) over the PR's set-based version. - userconfig.get_current_attachment: restore the cached lookup (GetAttachedCached) the PR regressed. - script.py: docstring class rename (ConfigSection).
GetConfigFile resolved the user config via the install-scope ConfigFilePath,
which points to ProgramData when the all-users marker is present. A
non-elevated Revit session then tried to write there ("access denied") and
ignored the real per-user APPDATA config.
- Resolve the writable user config from APPDATA (PyRevitPath) directly, so an
existing per-user config always wins (matches pyRevit's historical Python
discovery).
- When only an admin (ProgramData) config exists, probe actual writability
(FileInfo.IsReadOnly misses ACL denials) and open it read-only instead of
writing; otherwise seed it into the per-user location.
A value that fails JSON deserialization (e.g. escape-doubling corruption in older configs, such as a backslash-mangled environment.clones dict) threw out of GetValueOrDefault and aborted the entire config/section load. Fall back to the default instead, matching pyRevit's historical read tolerance. GetValue (non-default) still throws.
- ConfigurationService.SaveSection: stop RemoveOption on null properties so a partial-section save (PyRevitConfigs.Set* single-field records) no longer strips the other keys in that section. - ConfigurationService.GetSectionKeyValueOrDefault: return GetValueOrDefault instead of GetValue (was throwing on missing keys despite its name). - PyRevitConfigs.SetUTCStamps: write TelemetryUseUtcTimeStamps, not TelemetryStatus (copy-paste toggled telemetry on/off). - userconfig.apptelemetry_event_flags: guard None (hex(None) crashed at telemetry startup). - userconfig.reload(): restore the method (get_config(reload=True) raised TypeError); re-reads from disk. - configparser.get_option: tolerate non-JSON/legacy values instead of letting json.loads crash config reads.
Golden-file corpus + read/round-trip assertions over real-world config shapes (populated, corrupted clones, legacy formats, empty). Acceptance_* tests encode the Phase 2.1 symmetric-JSON target and are RED until it lands (C# string reads must be decoded, not JSON-quoted); the rest assert current Phase 1 behavior and pass. - Deploy pyRevitLabs.Json (Private=false in the Ini backend) to the test output so the suite runs standalone. - .gitignore: un-ignore the test .ini fixtures (matched by *.ini).
Comments authored during the port/fixes were describing why a change was made or referencing prior code. Reword them to state what the current code does (SaveSection null handling, GetConfigFile discovery, IsFileWritable, GetValueOrDefault fallback, ext-dir ordering, get_option non-JSON handling).
Strings are JSON-encoded once and decoded on read on both sides, so C# typed string access and the typed POCO sections no longer read back quoted. - IniConfiguration.GetValueImpl: deserialize strings (drop the raw-return special case). - IConfiguration/ConfigurationBase: add GetRawValueOrDefault/SetRawValue raw accessors; IniConfiguration stores/returns the value text unchanged. - configparser.ConfigSection and userconfig._SectionCompatWrapper: read/write via the raw accessors (json.loads / json.dumps once), removing the double-encode that fed escape-doubling. Golden-file string-contract tests now pass (18/18).
Repairs an existing config on first writable load: drops typed-section values that no longer parse to their declared type (e.g. an escape-doubled environment.clones dict), resets telemetry fields blown up by escape-doubling, then stamps [core] config_version so it runs once. - ConfigurationMigrator (version-gated, backs up before mutating). - PyRevitConfigs.GetConfigFile runs it on the writable user config only; admin/read-only configs are left untouched. - Remove the orphaned Python upgrade_user_config / heal_bloated_telemetry_fields; that work now lives in the migrator. - Golden-file tests: corrupt-value repair + idempotency.
…eError - TelemetrySection.AppTelemetryEventFlags int -> string: the field is a 128-bit hex string and overflowed Int32. PyRevitConfigs and userconfig pass it through as text (no hex parse/format). - ConfigSection.__getattr__ raises AttributeError for an absent option (was returning None), restoring hasattr / presence detection. - Golden-file fixture/test: large hex flags round-trip as a string.
- ConfigurationMigrator.Migrate returns a result (reset keys, backup path, backup-failed flag) and aborts when an existing file cannot be backed up, so it never mutates without a recoverable copy. - PyRevitConfigs logs the migration (Info) and each reset key (Warn), warns when migration is skipped for lack of a backup, and reports use of a read-only admin config (Info) since user changes are not saved.
GetValueOrDefault silently returned the default when a stored value failed to deserialize. Add a static ConfigurationDiagnostics sink (the assembly has no logger of its own); PyRevitConfigs routes it to logger.Warn, so a silently-defaulted value now leaves a trace even on read-only configs the migration cannot repair.
The migrator scanned and stamped a version once; a value corrupted after that stamp warned on every read but was never removed. Now the repair runs whenever an unreadable value is present on a writable config, independent of the version stamp. A config with nothing to fix performs no write, so a clean load still does not rewrite the ini.
Every config getter/setter rebuilt the service and re-ran migration on each call, and each engine re-read and re-saved the file. Introduce a process-wide cache so the loader, CLI, and script engines read one in-process instance, and hoist config discovery into the lightweight Configurations.Ini layer so the loader can share it without depending on the heavier pyRevitLabs.PyRevit or pyRevitLabs.Common assemblies. - Add PyRevitConfigStore: caches the built service by configuration name (default-name variants collapse to one instance); Reload invalidates. - Add PyRevitConfigService and PyRevitConfigPaths in Configurations.Ini: the default build factory (discovery, all-users fallback, seeding, migration) and file-location helpers, with no dependency on pyRevitLabs.Common. - PyRevitConfigs delegates GetConfigFile/ReloadConfig to the shared service and routes Configurations diagnostics to the pyRevit log. - ConfigurationService.SaveSection refreshes the typed section snapshots after a save so readers of the shared instance observe the write; userconfig save_changes captures the section snapshots before the sequential save so the refresh cannot drop pending edits. Tests: shared-store caching/reload, config-path discovery, and snapshot refresh on a real INI-backed service.
Point the loader's PyRevitConfig at the process-wide PyRevitConfigService instead of its own INI reader, so the loader, CLI, and Python engines share one in-process instance and one discovery path. PyRevitConfig becomes a thin adapter over IConfiguration with the same public surface and parsing semantics, using JSON-or-raw tolerant decoding so both migrated (JSON-encoded) and legacy (bare) values read correctly. ParseExtensionByName reads per-extension sections through the configuration; custom-path Load() stays non-shared for tests; ClearCache also drops the shared service cache. The old IniFile reader is left in place (now unused) next to the still-used PythonListParser, pending the in-Revit smoke test.
The loader now depends on pyRevitLabs.Configurations and .Configurations.Ini.
Exclude them and their INIFileParser dependency from the per-engine-folder
deploy so they load once from the bin/{netcore,netfx} root via
LoadAssembliesInFolder, matching how pyRevitLabs.Common is handled and avoiding
a duplicate/skewed load from two paths.
A per-key setter builds a sparse section POCO (e.g. new CoreSection { RocketMode
= x }), but SaveSection wrote every non-null property -- so the section's field-
initializer defaults (rocketmode, userextensions, port, sources, clones, ...)
overwrote or wiped sibling keys the caller never touched.
Move section read-defaults off field initializers: declare scalar defaults via
[DefaultValue] and supply an empty instance for collection properties, applied by
CreateSection on read. Unset properties are now null, so SaveSection's existing
null-skip writes only the keys a caller set -- and setting a property to its own
default value still persists, since it is explicitly non-null. Read-time default
values are unchanged.
…ervice Add ConfigParityTests: the loader's PyRevitConfig adapter and the shared ConfigurationService must decode the same canonical config file into identical values. The CLI and Python engines read through the same ConfigurationService, so loader/service parity transitively covers all readers and guards against a fourth reader ever drifting. Runs on net48 and net8.0-windows.
PyRevitConfig now reads through the shared configuration service, so the Win32 INI reader is unused. Remove it and move the still-used PythonListParser into its own file.
Contributor
|
Unable to trigger custom agent "Code Reviewer". You have run out of credits 😔 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a new shared configuration abstraction (pyRevitLabs.Configurations + pyRevitLabs.Configurations.Ini) and migrates the CLI + Python config layer to use it, with the goal of unifying INI parsing/defaults and reducing duplicated readers across components.
Changes:
- Added
pyRevitLabs.Configurationsabstractions + typed section POCOs, and an INI backend implemented onini-parser-netstandard. - Migrated CLI (
pyRevitLabs.PyRevit,pyRevitCLI) and Python config adapter (pyrevitlib/pyrevit/userconfig.py,coreutils/configparser.py) off the bespoke INI readers. - Added new xUnit test projects for the abstraction and the INI backend, and updated solution/build wiring.
Reviewed changes
Copilot reviewed 43 out of 45 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| pyrevitlib/pyrevit/userconfig.py | Refactors Python user_config to wrap the C# configuration service and typed sections. |
| pyrevitlib/pyrevit/script.py | Updates get_config() docstring type to the new ConfigSection wrapper. |
| pyrevitlib/pyrevit/revit/tabs.py | Hardens tab-coloring config reads against malformed types/values. |
| pyrevitlib/pyrevit/labs.py | Removes MadMilkman.Ini dependency and references the new Configurations assembly. |
| pyrevitlib/pyrevit/coreutils/configparser.py | Replaces Python-side INI parsing with a thin adapter over the C# configuration service. |
| dev/pyRevitLabs/tests/pyRevitLabs.Configurations.Tests/pyRevitLabs.Configurations.Tests.csproj | Adds new xUnit project for configuration abstraction tests. |
| dev/pyRevitLabs/tests/pyRevitLabs.Configurations.Tests/ConfigurationTests.cs | Adds baseline tests for IConfiguration behaviors. |
| dev/pyRevitLabs/tests/pyRevitLabs.Configurations.Tests/ConfigurationServiceUnitTests.cs | Adds (currently empty) unit test harness for ConfigurationService. |
| dev/pyRevitLabs/tests/pyRevitLabs.Configurations.Tests/ConfigurationServiceFixture.cs | Adds fixture for constructing ConfigurationService in tests. |
| dev/pyRevitLabs/tests/pyRevitLabs.Configurations.Ini.Tests/pyRevitLabs.Configurations.Ini.Tests.csproj | Adds xUnit project for INI backend tests. |
| dev/pyRevitLabs/tests/pyRevitLabs.Configurations.Ini.Tests/IniCreateFixture.cs | Adds test fixture that creates/deletes a temp INI file. |
| dev/pyRevitLabs/tests/pyRevitLabs.Configurations.Ini.Tests/IniConfigurationUnitTests.cs | Adds unit tests for INI configuration creation/builder validation. |
| dev/pyRevitLabs/pyRevitLabs.sln | Registers new projects and adds Any CPU/x86 configs and solution folders. |
| dev/pyRevitLabs/pyRevitLabs.PyRevit/pyRevitLabs.PyRevit.csproj | Removes MadMilkman.Ini packages and references the new Configurations projects; deploys INIFileParser if present. |
| dev/pyRevitLabs/pyRevitLabs.PyRevit/PyRevitExtensions.cs | Migrates extension enable/disable and extension path handling to IConfigurationService and section POCO saves. |
| dev/pyRevitLabs/pyRevitLabs.PyRevit/PyRevitConsts.cs | Minor whitespace/comment formatting changes only. |
| dev/pyRevitLabs/pyRevitLabs.PyRevit/PyRevitConfigs.cs | Replaces the old config reader with ConfigurationBuilder + INI configuration sources; adds read-only detection via write-probe; threads optional “revitYear” layer through setters. |
| dev/pyRevitLabs/pyRevitLabs.PyRevit/PyRevitConfig.cs | Deletes the old MadMilkman-backed config reader. |
| dev/pyRevitLabs/pyRevitLabs.PyRevit/PyRevitClones.cs | Migrates clone registry persistence to typed EnvironmentSection. |
| dev/pyRevitLabs/pyRevitLabs.Configurations/Sections/TelemetrySection.cs | Adds typed telemetry section definition. |
| dev/pyRevitLabs/pyRevitLabs.Configurations/Sections/RoutesSection.cs | Adds typed routes section definition. |
| dev/pyRevitLabs/pyRevitLabs.Configurations/Sections/EnvironmentSection.cs | Adds typed environment section definition. |
| dev/pyRevitLabs/pyRevitLabs.Configurations/Sections/CoreSection.cs | Adds typed core section definition and defaults. |
| dev/pyRevitLabs/pyRevitLabs.Configurations/pyRevitLabs.Configurations.csproj | Adds multi-targeted Configurations project (net48/net8.0) and InternalsVisibleTo for tests. |
| dev/pyRevitLabs/pyRevitLabs.Configurations/Extensions/ConfigurationExtensions.cs | Introduces placeholder extension class (currently empty). |
| dev/pyRevitLabs/pyRevitLabs.Configurations/Exceptions/ConfigurationSectionNotFoundException.cs | Adds custom exception for missing sections. |
| dev/pyRevitLabs/pyRevitLabs.Configurations/Exceptions/ConfigurationSectionKeyNotFoundException.cs | Adds custom exception for missing keys. |
| dev/pyRevitLabs/pyRevitLabs.Configurations/Exceptions/ConfigurationException.cs | Adds base configuration exception type. |
| dev/pyRevitLabs/pyRevitLabs.Configurations/Constants.cs | Adds internal constants for env-related section/key names. |
| dev/pyRevitLabs/pyRevitLabs.Configurations/ConfigurationService.cs | Adds the configuration service for layering + section (de)serialization via attributes. |
| dev/pyRevitLabs/pyRevitLabs.Configurations/ConfigurationName.cs | Adds internal record to track layered config names/order. |
| dev/pyRevitLabs/pyRevitLabs.Configurations/ConfigurationBuilder.cs | Adds builder for composing layered IConfiguration sources into a service. |
| dev/pyRevitLabs/pyRevitLabs.Configurations/ConfigurationBase.cs | Adds base class implementing common IConfiguration behaviors + tolerant reads. |
| dev/pyRevitLabs/pyRevitLabs.Configurations/Attributes/SectionNameAttribute.cs | Adds attribute for binding POCOs to INI section names. |
| dev/pyRevitLabs/pyRevitLabs.Configurations/Attributes/KeyNameAttribute.cs | Adds attribute for binding POCO properties to INI key names. |
| dev/pyRevitLabs/pyRevitLabs.Configurations/Abstractions/IConfigurationService.cs | Adds service contract for layered configurations + typed sections. |
| dev/pyRevitLabs/pyRevitLabs.Configurations/Abstractions/IConfiguration.cs | Adds configuration backend contract (read/write/serialize). |
| dev/pyRevitLabs/pyRevitLabs.Configurations.Ini/pyRevitLabs.Configurations.Ini.csproj | Adds INI backend project targeting net48/net8.0 with ini-parser and pyRevitLabs.Json reference. |
| dev/pyRevitLabs/pyRevitLabs.Configurations.Ini/IniConfiguration.cs | Implements IConfiguration using ini-parser with JSON-based value serialization and some legacy parsing logic. |
| dev/pyRevitLabs/pyRevitLabs.Configurations.Ini/Extensions/IniConfigurationExtensions.cs | Adds builder extension for registering INI configurations. |
| dev/pyRevitLabs/pyRevitCLI/Resources/UsagePatterns.txt | Extends pyrevit configs usage patterns to accept optional <revit_year>. |
| dev/pyRevitLabs/pyRevitCLI/PyRevitCLIExtensionCmds.cs | Threads revit version layer into extension enable/disable calls. |
| dev/pyRevitLabs/pyRevitCLI/PyRevitCLI.cs | Threads <revit_year> through config setters and extension toggle path. |
| dev/Directory.Build.targets | Adds NetFolder mapping for net8.0 to netcore output folder. |
| .gitignore | Un-ignores the new pyRevitLabs.Configurations.Ini directory from the existing *.ini ignore rule. |
- userconfig: config_file reports the service-resolved path, not the fixed install-scope path - userconfig: _SectionCompatWrapper.get_option tolerates non-JSON values instead of raising - userconfig: fix remove_section body indentation - versionmgr/upgrade: remove dead upgrade_user_config + telemetry-heal helpers (and the now-unused constants/imports) - tabs: resolve tab-style index through a tolerant fallback to the default index on malformed/out-of-range config - Configurations: make the IConfiguration Type-based overloads public - Configurations.Ini: parse hex integers as Int64 so long targets do not overflow - Configurations: pass (keyName, sectionName) to ConfigurationSectionKeyNotFoundException so its fields are correct - Configurations.Ini: fix conigurationName parameter typo - tests: drop duplicate apptelemetry_event_flags SetValue in fixture
- Config-file discovery regex: `.*[pyrevit|config].*\.ini` used a character clas,. Use `(pyrevit|config)` instead. Fixed in both resolvers (PyRevitConsts and PyRevitConfigPaths). - extension.json bool parsing: BuiltIn/DefaultEnabled/ RocketModeCompatible called bool.Parse directly, throwing on a missing field or non-"true"/"false" string. Parse defensively with per-field defaults (builtin=false, default_enabled=true, rocket_mode_compatible=false)
Reconcile the config-store rewrite with develop's install-scope/config-split (pyrevitlabs#3452) and loader-auth (pyrevitlabs#3461) work. Kept the branch's config-store architecture; ported seedshippeddefaults to the new IConfigurationService API and re-added parser PyRevitConfig.ExtensionLookupSources for the auth path. Dropped the pyrevitlabs#3441 admin split-config migration (depends on the deleted PyRevitConfig class) as a tracked follow-up. Builds on net48+net8.0; config and parity test suites pass.
…g store - SetSectionKeyValue now persists to disk (SaveConfiguration + snapshot reload), fixing silent no-ops in `pyrevit configs` and extension enable/disable - extension search-path and lookup-source getters save only when normalization actually changed the stored list, removing a write side effect from a read - SetLoggingLevel(Debug) writes verbose=true to match the Python setter - SaveSection no longer writes back a declared default that isn't already stored, so a single edit via the settings dialog stops materializing every core/routes/telemetry default into the user config - telemetry path/url getters return string.Empty instead of null
… reads
Seven defects that silently lost user settings, misread them, or aborted
session load.
- Legacy Python bools ("True"/"False") failed JSON parsing, so they read as
their default and were then deleted by the migrator. Now tolerated in
IniConfiguration alongside the hex-int and bare-string fallbacks.
- Legacy lists and maps holding unescaped Windows paths hit the same gap
(backslashes are invalid JSON escapes), wiping every extension search path
and clone entry on first load. Container values retry with backslashes
escaped.
- pyRevit_config.<year>.ini matched the generic config-name pattern and sorted
ahead of pyRevit_config.ini, so it was adopted as the main config for every
Revit version. Discovery takes an exact canonical-name match first and
excludes version-suffixed files from the fallback.
- ConfigurationService rebuilt its typed snapshots only on explicit reload, so
a raw SetRawValue left them stale and the next SaveSection wrote the stale
copy back over the new value. Snapshots now rebuild lazily off a
per-configuration revision counter.
- ConfigSections.get_section returned a live section for any name, so a missing
section never raised. It raises AttributeError again, and add_section
materializes the section via a new IConfiguration.AddSection so add-then-get
still works.
- PythonListParser dropped legacy single-quoted lists with unescaped paths,
returning an empty list. Parsed directly now, backslashes literal so C:\temp
does not become a tab.
- An unset config option reaches the runtime env dictionary as null, and
EnvDictionary unboxed it straight to bool. A config without a [telemetry]
section therefore threw NullReferenceException from setup_hooks and aborted
session load before the ribbon was built. EnvDictionary now matches on type
rather than casting, and the telemetry booleans carry declared defaults.
Adds 33 tests; 110 passing across the Ini, Configurations, parser, and Python
suites.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Remove tests that pinned the initial config-store scaffolding rather than lasting behavior: - delete the empty ConfigurationServiceUnitTests and its all-throwing ConfigurationServiceFixture stub - delete PyRevitConfigPathsTests (superseded by ConfigDiscoveryTests), folding its two unique config-name-pattern cases into ConfigDiscoveryTests - drop the unasserted StartUp() seeding from ConfigurationTests, keeping the five backend-contract fact
…vice
Consolidate every config reader onto one decode path. The loader's
PyRevitConfig re-implemented value decoding in parallel with
pyRevitLabs.Configurations, risking drift between what the loader, CLI,
and Python read from the same file. It now delegates to the shared
IConfigurationService, and the duplicated decode/legacy logic moves into
the service where all readers share it.
Service:
- Add ExtensionSection + IConfigurationService.GetExtensionSection so the
dynamic "{name}.extension"/".lib" sections have a typed accessor.
- Decode List<string> across every historical encoding (canonical JSON,
unescaped-backslash JSON, and the legacy Python single-quoted literal)
in the INI backend, via the shared LegacyListFormat helper.
- ConfigurationMigrator canonicalizes legacy single-quoted lists to JSON
on writable configs (reported as ConvertedKeys); idempotent because the
canonical form is not re-detected as legacy. Read-only configs still
decode tolerantly on read, now surfaced via ConfigurationDiagnostics.
Loader:
- PyRevitConfig is now a read-only facade: core/telemetry/extension/list
getters delegate to the shared service; the parallel decode/encode
helpers and unused setters are removed.
- Restore the rocketmode default to true, matching the long-standing
ConfigsRocketModeDefault and the service/Python/CLI. The loader adapter
had regressed it to false; absent the key, rocket mode is on again.
Tests:
- Move PythonListParser coverage into the Ini backend (ListDecodingTests);
add MigrationCanonicalizationTests and ExtensionSectionTests.
- Trim loader setter round-trip tests (the loader no longer writes config).
Typed core/routes/telemetry setters mutated a rebuildable snapshot, so edits were dropped when an intervening write bumped the store revision mid-save. Add ApplySection<T> (write-through, no flush) and route typed setters through it; save_changes() flushes once. SaveSection/CLI behavior unchanged.
Boolean, integer, and string keys in the core/routes/telemetry sections
had no [DefaultValue], so an unset key read back as null. That null then
surfaced through the Python facade — e.g. min_host_drivefreespace read as
None, populated the Settings field with the string "None", and failed
int() parsing on save ("Minimum free space value must be an integer").
Declare the defaults on the section properties themselves (false / 0 / "")
so the configuration service materializes them on read for absent keys —
matching what the PyRevitConfigs facade already coerced to. UserLocale is
intentionally left defaultless (null still means "auto-detect from the
Revit UI language"), and routes.host / apptelemetry_server_url keep their
null default to match the facade.
- IniConfiguration: create the parent directory on save; a fresh install hit DirectoryNotFoundException stamping the schema version. - LegacyListFormat: stop treating "[]" as legacy. It is the canonical empty list, so every load rewrote the config and left another .bak. - PyRevitExtensions: read userextensions as List<string> so it goes through DecodeStringList; string[] resolved legacy values to empty and the next path registration overwrote the stored value. - ApplySection: compare against the write target, not the first config that answers, so a save to an override config no longer no-ops. - EnableAppTelemetry: set AppTelemetryStatus, matching EnableTelemetry. - PyRevitCLI: pass revitVersion by name; positionally it bound to apptelemetryServerUrl and wrote "Default" over a configured URL. - _SectionCompatWrapper.get_option: accept default_value and decode raw through the ConfigSection ladder. The renamed keyword broke the shipped Extensions smartbutton (dropping offline paths) and the typed read returned stored booleans as the truthy text "False". - ConfigSection.add_subsection: register the section so it is visible
Five defects found in code review, each able to break config loading or silently discard user settings. - Read INI files the way the Python readers do. ini-parser's defaults throw from the constructor on a '#' comment, a duplicated key or section, or a line without an assignment, so an existing config carrying any of those failed to load at all and never reached the migrator that would repair it. - Refuse writes to a read-only (admin lockdown) config rather than applying them to memory and dropping them at flush time, which left the CLI reporting success and the live state disagreeing with the file. - Invalidate the loader's cached PyRevitConfig when the shared store rebuilds, so a Python user_config.reload() no longer leaves the loader reading a detached, pre-reload configuration. - Build each configuration name once under concurrent first access; the factory seeds, migrates, and backs up files, which GetOrAdd may run on several threads at once. A failed build is evicted, not cached. - Throw on an undecodable list value instead of returning an empty list, which silently dropped every third-party extension and hid the damage from the migrator. The loader tests injected a config by reflecting into a private static field, which the cache fix closes by design; they now use the PyRevitConfigStore.SetFactory seam
Fill in XML docs for the Configurations/Configurations.Ini public surface (interfaces, section schema with units and defaults, exceptions), and turn on GenerateDocumentationFile with CS1591 as an error in both projects. Also drop change-rationale and stale comments across the branch.
- bincache defaulted to false in CoreSection, contradicting PyRevitConsts.ConfigsBinaryCacheDefault, so anyone who never set the key got the slower ASCII extension cacher. - save_changes raised on a failed write, aborting session load from the telemetry setup path and throwing out of the Settings dialog. - reload() replaced the service, orphaning the ConfigSection objects already handed to scripts: their edits landed in the replaced store and were lost on save. ConfigSection and ConfigSections now take either a live object or a zero-argument accessor and resolve per access, so a section follows a reload.
The split-admin merge treated presence of the `clones` key as proof that
the machine config still held its clone registry. Unregistering the last
clone writes `clones = {}`, so a machine install whose registry had been
emptied never recovered the clones still recorded in the per-user config
— the exact split-config case the migration exists to repair.
Gate the backfill on the decoded clone count instead, on both sides. A
value that is present but will not decode counts as unknown, so the merge
leaves unreadable user data alone rather than mistaking it for empty.
…on access - Loader booleans now accept exactly what the shared service accepts, so `loadbeta = yes` no longer reads as enabled to the loader and disabled to the CLI and Python. - Split-admin repair renames the per-user config aside once its settings reach the machine config. The merge previously re-ran on every load, so a per-extension section an admin removed from the machine config came back on the next start. An incomplete repair leaves the file and retries. - user_config.core/routes/telemetry fall back to a raw config option for a name the typed schema does not declare, and gain has_option, restoring the access every other section allows.
The two Configurations xunit projects were in the solution but no workflow ran them, and pyRevitExtensionParserTester was built by the CI pipeline with its results discarded. Adds two ci.yml steps: both xunit projects, plus a filtered run of the config fixtures in the parser tester. The rest of that suite has never run here, so enabling it wholesale is left as its own change. That surfaced a test which had never passed: LoggingLevel_Debug_MapsToDebugAndClearsVerbose asserted that setting Debug clears verbose, contradicting LoggingLevelVerboseFlag, where debug implies verbose and both flags are written together. Renamed and corrected. Adds RealBackendContractTests to the Python bridge suite, pinning the IConfiguration behaviors _FakeConfiguration models against the real INI backend so the fake cannot drift from the contract it claims to model. Trims Defaults_AppliedOnRead_WhenKeyAbsent to one case per type and per section rather than restating every declared default.
ConfigurationMigrator flagged the three telemetry fields only when a stored value exceeded 8192 characters, but the corruption this healing exists for never reaches that size. An emptied field that went through repeated escape doubling lands at 14-16 characters of quote and escape artifacts. The typed-section scan did not catch it either: all three fields are declared string, and the INI reader returns the raw text rather than throwing when a value fails to decode, so an affected config was carried forward unrepaired. Detection now considers shape as well as length. A value whose entire content is quote, escape, and slash artifacts carries nothing recoverable and is reset. Slashes count as artifacts only for the two URL fields, where the separators survive the blow-up; in a file directory a slash is part of the path. A value with no quote at all is a legacy bare path or URL, and the canonical empty string is not corruption, so both are left alone. The heuristic is jmcouffin's from pyrevitlabs#3536, ported into the migrator that owns this healing here in place of versionmgr.upgrade.heal_bloated_telemetry_fields. Adds a golden set of the reported values plus the short legitimate values that must survive, so the two cases stay distinguishable in CI. Fixes pyrevitlabs#3534.
Two read APIs in PyRevitExtensions self-healed by writing, which this branch's stricter write path turns from a harmless no-op into a fault. - GetRegisteredExtensionSearchPaths and GetRegisteredExtensionLookupSources saved a normalized list whenever it differed from the stored one. A read-only configuration now refuses a write up front, so on an admin-locked install a single stale entry made `pyrevit env`, `pyrevit extensions`, and the paths and sources subcommands throw instead of printing. - The stored search-path list is now read verbatim, separately from the resolved one, and as a copy since the typed section is a cached snapshot. Resolution expands environment variables for use, and persisting that back rewrote %APPDATA%\ext to an absolute path on the first read, so a portable entry survived one load and the loader, which does not expand, disagreed with the CLI until it did. Registration keeps a variable in the caller's text; unregistration matches the resolved form, so such an entry is still removable by the path it expands to. A read no longer prunes entries that do not currently resolve; every consumer resolves before use, so an offline share is not dropped from the config. Regression tests cover all four cases.
user_config resolved the shared IConfigurationService once at import and held it for the life of the process. The loader clears its parser caches on every session load, which drops the store's cached service, so from that point the Python facade served an orphaned instance while every labs call built a fresh one. Reads went stale, and save_changes() flushed the orphan's entire in-memory state over the file -- discarding whatever the labs side had registered in between, such as a newly added extension path or a registered clone. Resolve the service through the store on each access instead, which is the guard the loader's own PyRevitConfig.Load() already applies via ReferenceEquals. The store caches the built service, so this costs a dictionary lookup rather than a rebuild, and the previously resolved service is kept only as a fallback for a store that cannot be reached. is_readonly and config_type become properties for the same reason: both were computed at import, and a rebuilt service can resolve a different config tier.
ApplySection throws on an admin-locked config, so setup_telemetry's unconditional writes aborted session load. The dynamic path had the opposite problem: SetRawValue accepted writes that save_changes then never flushed. Skip every write path when the service is read-only, and have remove_option report that it removed nothing. Assigning None to a typed property mutated the process-wide section snapshot without persisting anything. It is now a no-op; remove_option clears a key. Pins the contract with read-only ApplySection and raw-write cases in the CI-gated Ini suite, plus read-only cases in the Python wrapper tests. Pins the contract with read-only ApplySection and raw-write cases in the CI-gated Ini suite, and covers the typed write-through, None, and read-only paths in the Python wrapper tests.
Subsections are stored as sibling sections under a dotted name, so remove_section left them behind. script.reset_config() and remove_pkg_config() reported a clean removal while the next install of the same tool silently inherited the stale values. Sweep the dotted prefix before removing the parent, which also reaches subsections whose parent header no longer exists.
get_option returned the raw text for a bare True/False, so a legacy disabled = False read as the truthy string "False". extpackages computes is_enabled as `not config.disabled`, which made the package read as disabled while the C# reader parsed the same key correctly. Migration does not reach these keys: it canonicalizes only the typed sections, so per-extension values keep the legacy spelling indefinitely. Decode bare bool tokens case-insensitively, matching bool.TryParse on the C# side, from one decoder shared by both bridges. Values written through set_option are JSON-quoted, so a string that spells "True" is unaffected.
The section:option reader treated an empty stored value as absent, printing "is not set" for a key explicitly stored as "". A value could be written and then never read back, and the empty-vs-removed distinction the config store relies on was invisible from the CLI. Test for null instead: the reader returns null only when the section or key is missing, so a present-but-empty value now prints.
…path routes.host lost its empty-string default, so an unset key resolved to null. The routes server passes the value straight to a socket bind, which rejects null where "" binds every available interface; activate_server caught the TypeError and unregistered the server, so routes never started on any config that had not set a host explicitly. Declare the default on the section. Reads materialize "" in memory, and the write path already declines to persist a value equal to the declared default, so an unset host stays out of the file rather than gaining an empty entry. SetupConfig lost the EnsurePath on its template branch, so `pyrevit config --from=<template>` threw DirectoryNotFoundException on a machine where %APPDATA%\pyRevit did not exist yet — the bootstrap case the command exists to serve. Create the directory before writing, inside the existing try so a failure still surfaces as PyRevitException.
Remove test cases that cannot fail: the ported IConfiguration scaffolding, an assertion that a factory returns non-null, and duplicate coverage of the diagnostic sink and typed-section load. Leaving one mutator of the static ConfigurationDiagnostics.Warn also removes a race between two xunit classes that run in parallel. Give the parity fixture values that contradict their section defaults. Three of its assertions previously stored the same value the reader falls back to, so they passed against a property that never reached the config at all. Correct the ConfigParityTests docstring and name: the loader and CLI read the same ConfigurationService, but the Python bridge decodes raw strings on its own side and is covered by test_config_roundtrip.
The override ported from pyrevitlabs#2482 only ever had a write half: setters took a Revit year and wrote pyRevit_config.<year>.ini, but every getter, the loader, and the migrator read the base config. The grammar was ambiguous too -- `[<revit_year>]` followed an optional value positional, so `pyrevit configs startuptimeout 2025` set the timeout instead of reading the 2025 override. Remove it and collapse the abstraction behind it: IConfigurationService now serves exactly one configuration. Names, layered reads, and the this[name] indexer give way to a single Configuration property; PyRevitConfigStore caches one Lazy; the 35 PyRevitConfigs setters lose their revitVersion parameter; UsagePatterns returns to its develop state. Configurations tests 16 -> 14 and 91 -> 89, dropping the name-keying and override-write cases.
The develop merge brought in two callers of PyRevitConfigParser, which this branch had replaced with ConfigSections over the C# config service. settings_window imports it at module scope, so every import of that module raised ImportError, not just the custom_config path that uses it; the FilterLegend pushbutton failed the same way. Both want a private ini under appdata rather than the shared pyRevit config, so add open_config_file() instead of repointing them at the config service. Supporting: reference pyRevitLabs.Configurations.Ini from pyrevit.labs, and add ConfigSections.save(), since option writes do not flush on their own. Adds StandaloneConfigFileTests over the new helper
The migrator rewrote legacy single-quoted lists but not dicts, so a
config carrying the Python-repr clones map kept the old spelling while
being stamped config_version = 1 -- a claim that the file is canonical,
which a later migration would be entitled to trust.
Add LegacyDictFormat, mirroring LegacyListFormat: hand-parsed so
unescaped Windows backslashes survive, and declining any literal that
contains a double quote, plus the canonical "{}", so a canonical value
is never detected as legacy and rewritten on every load.
Adds three cases to MigrationCanonicalizationTests and folds the empty
dict into the canonical-empty-container case
The store selected the %ProgramData% tier for every process on an all-users install, then resolved the path through GetActiveConfigFilePath(), which returns the %APPDATA% path for anything unelevated. Selection and resolution disagreed, so a Revit session ran on a per-user config the store believed was the machine config. The split-admin repair then renamed that file aside on every load and the next load reseeded it from %ProgramData%, discarding the prior session's [core], [routes], and [telemetry] edits and leaving another .split-admin.<ts>.bak behind each time. Take elevation as a selection input: only an elevated process (installer / admin CLI) resolves to %ProgramData%, matching the policy develop settled in pyrevitlabs#3512 and pyrevitlabs#3523. Standard users on an all-users install seed and keep a writable per-user copy. Drop the write probe from the ladder. Lockdown is the DOS ReadOnly attribute alone; folding an ACL denial into it handed every standard user on an admin install a read-only config whose saves were silently discarded (pyrevitlabs#3504). Retire the split per-user config only when the merge actually moved a setting into the machine config, and run the repair only from an elevated process. The merge carries the clone registry and extension sections alone, so a config that contributed nothing still holds the only copy of its other sections.
decode_option_value tried canonical JSON and the Python single-quoted form, but neither accounts for the unescaped backslashes legacy configs wrote into Windows paths — JSON rejects them as bad escape sequences. The value fell through to the raw-text fallback, so a list of extension paths decoded to the literal string "['C:\Users\...']". Extensions.smartbutton then iterated that string per character and wrote single-character "paths" back to userextensions. Try each spelling the C# reader tries, in the same order: canonical JSON, the single-quoted legacy form, then both with backslashes escaped. The migrator canonicalizes only the single-quoted form, so the double-quoted spelling the C# reader tolerates persists in configs indefinitely and both readers have to agree on it.
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.
Edit: The original description outlined 3 phases. All work is complete and I've rewritten this description.
Summary
pyRevit has three independent configuration readers for
pyRevit_config.ini, each with its own parsing, defaults, and quirks that have drifted apart over time:pyrevit.coreutils.configparser)pyRevitLabs.PyRevit)pyRevitExtensionParser)None of them share a parsed result, so the file is read from disk once by the C# loader and then again for every startup script and smartbutton engine — at least five times for a basic install, at 200–500ms each.
This branch replaces all three with one C#-owned configuration store shared by the loader, CLI, IronPython, and CPython, and quarantines legacy INI parsing behind a version-stamped migration that repairs a config in place.
It is a port of @dosymep's #2482 onto current
develop, reconciled with everything that changed since that PR was opened, and extended with install-scope awareness, migration, and loader integration. .Change size
The configuration service
New
pyRevitLabs.Configurationsassembly:IConfigurationService/IConfigurationwith attribute-bound typed POCO sections ([SectionName]/[KeyName]/[DefaultValue]) for[core],[routes],[telemetry],[environment], and per-extension sections (Sections/). Defaults live on the section schema instead of being restated by each reader.GetValueOrDefaultreturns the default when a single stored value fails to decode, so one malformed key does not abort the whole section load.EnsureSnapshots), so a reader never observes state older than the last write and a snapshot-only edit cannot be silently dropped.EnsureWritable) rather than accepting them into memory and discarding them at flush time.The INI backend
pyRevitLabs.Configurations.Inireads and writes UTF-8 without a BOM so Python'sconfigparsercan read the same file.SetValueImpl) and decoded once on read (GetValueImpl), on both the C# and Python sides. This is what closes the escape-doubling growth in #3334.True/False, hex integers (0x8000000), bare unquoted strings and Windows paths, and Python single-quoted list literals. The single-quoted list form is defined once inLegacyListFormatand shared by the read path and the migrator so both interpret it identically;LegacyDictFormatis its counterpart for the clone registry, which the tolerant JSON read already accepts and the migrator uses to recognize what to canonicalize.Migration and self-healing
ConfigurationMigratorruns on any writable load, stamped with a[core] config_versionkey, so corruption introduced after first run still heals.Several recent GitHub issues trace back to config parsing. Centralizing the repair logic in a single migrator gives those fixes one home — one that can be extended for future issues, or trimmed once a fix is no longer relevant.
"", and legacy bare unquoted values are left alone.Migrate).Consumers
CLI (
PyRevitConfigs.cs) — all config access goes throughIConfigurationService; the bespoke CLI parser (pyRevitLabs.PyRevit/PyRevitConfig.cs, −176 lines) is deleted.Loader (
PyRevitConfig.cs) — now a read-only facade overIConfiguration, and the standalone Win32IniFile.csreader is deleted (−460 lines). Core, telemetry, and per-extension values decode through the service so the loader interprets them identically to the CLI and Python. The loader's cached instance is held only while it still wraps the configuration the shared store hands out, so a reload requested by any host is observed (Load).ClearAllCachesalso resets locale tracking so a reload cannot re-trigger itself mid-parse.Python (
userconfig.py,coreutils/configparser.py) — a thin adapter over the same service, dropping theMadMilkman.Inidependency. Typed section properties are written through to the shared store on assignment, andsave_changesflushes once.versionmgr/upgrade.pydrops to the 4.8.5 temp-file cleanup now thatConfigurationMigratorowns telemetry bloat-healing. Most of what remains is passthrough — 34 ofuserconfig.py's 40 accessors are now one-line delegations — and can be deprecated in a future release once extensions move to the typedsection.nameform.Behavior and API changes
apptelemetry_event_flagsis stored as a string, not an int — the 128-bit hex bitmask overflowsint(TelemetrySection). Legacy0x…values still read.[core] config_versionkey and, on first repair, leaves a.v0.<timestamp>.bakalongside it.%ProgramData%is the writable target only for an elevated process (installer, admin CLI). Standard users resolve to a per-user config under%APPDATA%, seeded from the machine config, which is the policydevelopsettled in #3512 and #3523. A machine config a process cannot write is not treated as a lockdown; only the DOS ReadOnly attribute is (pyrevit configs seed --lock).%APPDATA%config is their own active config rather than a stray half of a machine install. When it moves the clone registry or a missing extension section into the machine config, the per-user file it consumed is renamed topyRevit_config.ini.split-admin.<timestamp>.bak. A per-user config with nothing left to contribute is left where it is. Note that the merge carries the clone registry and extension sections only, so the retired copy is where that user's[core],[routes], and[telemetry]values remain.Breaking changes
The surface extensions actually use is unchanged:
user_config,CONSTS, the snake_case accessors,get_section/add_section/has_section/remove_section,save_changes,reload, andget_option/set_option/has_option/remove_optionon every section. No caller in this repository uses any removed name, and a survey of all 25 extensions inextensions/extensions.jsonfound none either — only two use the config service at all, both exclusively through the retained surface.Summary of internals removed:
coreutils.configparser— the two parser classesPyRevitConfigParser/PyRevitConfigSectionParser, replaced byConfigSections/ConfigSectionover the shared service. A tool that kept its settings in its own ini rather than the shared config usedPyRevitConfigParserfor it;open_config_file(path)covers that case, returningConfigSectionsover any ini path.userconfigmodule level — the path placeholders that were all empty strings (CONFIG_FILE,USER_CONFIG_FILE,ADMIN_CONFIG_FILE,LOCAL_CONFIG_FILE; useuser_config.config_file, the path the service actually resolved), and the discovery helpersfind_config_file/verify_configs— discovery, seeding, and repair now happen insidePyRevitConfigService.PyRevitConfig—get_config_version()(unrelated to the new[core] config_versionmigration stamp), andget_config_file_hash(). Its constructor now takes the configuration service rather than a file path.versionmgr.upgrade—upgrade_user_config/heal_bloated_telemetry_fields, now owned byConfigurationMigrator, including the short-malformed-value healing added by #3536.upgrade_existing_pyrevit(), the only entry point called anywhere, is unchanged. Two differences are deliberate rather than oversights:upgrade_user_config's decode/re-encode sweep over every option in every section is not ported. Re-encoding a value the decoder had already mangled is the escape-doubling mechanism itself, so the sweep is what produced the corruptionheal_bloated_telemetry_fieldsexists to repair. The migrator instead rewrites only keys it has positively identified as legacy or unreadable, which removes the write-amplification path entirely.''. Identical for the two fields declaring a""default;apptelemetry_server_urldeclares none, so it reads back as null rather than an empty string.Testing
The .NET suites below now run in CI (
Run configuration unit testsandRun config parity testsinci.yml), so a regression fails the pipeline rather than waiting on a manual check.pyRevitLabs.Configurations.Tests(xUnit, 14 tests) — extension-section resolution, andPyRevitConfigStorecaching, concurrency, failed-build eviction, and reset.pyRevitLabs.Configurations.Ini.Tests(xUnit, 104 cases) — golden-file fidelity corpus round-tripping four real-world config shapes (populated,corrupted_clones,legacy_values,empty), list decoding across every historical encoding, malformed-file tolerance, read-only write guards, migration canonicalization for both legacy lists and the clone registry dict, telemetry-artifact repair, snapshot freshness, and discovery. The selection ladder is covered across both install scope and process elevation, including that a machine config a standard user cannot write resolves to a writable per-user copy rather than a read-only one (#3504). The split-admin repair is driven end to end rather than through its merge step alone, so the cases that decide whether the per-user config is retired — nothing merged, settings moved, promotion, and an inert second pass over an already-repaired install — are each asserted.ConfigParityTestsandPyRevitConfigsFacadeTests(NUnit, 17 tests, run frompyRevitExtensionParserTesterby filter) — get-after-set round-trips through the CLI configuration facade, each verified against a fresh read of the file so a write is proven to reach disk; and the loader's config surface held to the shared service, over a fixture whose every value contradicts its section default so an assertion cannot pass on a property that never reaches the config.test_config_roundtrip.py(66 tests) — Python-bridge round-trips, runnable under IPY2, IPY3, and CPython. 56 are hermetic, running against a dict-backed fakeIConfiguration; these include the decoding of container values whose Windows paths carry unescaped backslashes, which JSON rejects and both readers therefore have to retry escaped, asserted down to the failure it caused — a path list that decodes to a string and is then iterated one character at a time. The remaining 10 need the real INI backend over a temp file and skip when the labs assemblies are not loadable:RealBackendContractTests(7) re-runs the load-bearing assertions against it so the fake cannot drift from the contract it models, andStandaloneConfigFileTests(3) coversopen_config_file, the entry point for a tool's own ini file.test_config_*module inside a live Revit session, exercising the Python config bridge against the real store.Out of scope, and follow-up
YamlDotNetdependency and no format-dispatch left dangling. TheIConfigurationabstraction is retained, so another backend can be reintroduced through it if a use case appears.pyrevit configs rocketmode enable 2025→ a versionedpyRevit_config.2025.ini). Only the write half of the feature was implemented. Adding full support should be a seperate PR. Rather than ship a command that reports success and does nothing, the whole feature has been removed.dotnet test tests/Build.Tests.csproj, which covers the build pipeline rather than product code.pyRevitExtensionParserTester(~297 NUnit tests) is built on every run with its results discarded — this PR executes only the 17 config-related ones — andpyRevitLabs.UnitTests(31 MSTest) is referenced from the solution only, and touches install-scope and Revit addon paths, so it needs an audit for which cases are hermetic first.